exercism / sml

Exercism exercises in Standard ML.
https://exercism.org/tracks/sml
MIT License
26 stars 34 forks source link

Script to redeploy testlib.sml to all exercises on changes. #206

Closed guygastineau closed 2 years ago

guygastineau commented 2 years ago

Okay, so I decided it is better to factor this redeploy script out of #205 .

I also went ahead and refactored to use the Makefile for ease of use and hopefully for better portability across operating systems. GH inscluded my commit message below.


The directory name sml-bin makes the most sense to me for these sml "scripts". Adding them to the makefile also makes it even easier to use these than it is when using "#! /usr/bin/env -S poly --script" as a shebang line. Consequently this should be portable to any Windows system that has GNU make installed, so it is probably better than the shebang. In the code I used the path library to help make things OS portable, but I haven't ever used SML on Windows, so it is untested for that enironment.

rainij commented 2 years ago

@guygastineau this PR is related to issue https://github.com/exercism/sml/issues/110. Not sure if you are aware of it.

guygastineau commented 2 years ago

I hadn't seen #110. Thank you for referencing it for me.

guygastineau commented 2 years ago

Concerning not using bin for sml "scripts". When I see something in a bin dir I expect that I can execute it from the project root like this bin/something. Following the principle of least astonishment, I think it is best for such sml code to live in another directory. It could be named very many things including code, sml, etc..., but I definitely think all files in bin should be executable.

guygastineau commented 2 years ago

I'll set my editor for using 2 spaces in sml-mode, and then I can reformat and push the changes.

rainij commented 2 years ago

Concerning not using bin for sml "scripts". When I see something in a bin dir I expect that I can execute it from the project root like this bin/something. Following the principle of least astonishment, I think it is best for such sml code to live in another directory. It could be named very many things including code, sml, etc..., but I definitely think all files in bin should be executable.

@guygastineau sounds reasonable to me. But I wonder if sml-bin is a good name then. Because now I would think that sml-bin still contains "executables", which happen to be written in SML. Maybe sml-scripts? But actually I never thought about the question what a "script" really is^^. Similar vor "bin" - sounds like machine code to me but I usually don't care if it is machine code ... .

In any case, it is interesting that "this" problem arises from the desire to support Unix and Windows. I am not very happy that this is the reason for the additional folder, but I see your point.

So in the end I have some doubts but I would be OK with or without the additional folder. I have a slight preference to still put the script into 'bin' since although I understand your reasoning I think it still has a drawback. Others not knowing your reasoning (like me before reading your justification) might now struggle with the question where to put the next script. I must admit that I have no clue what others might expect over "bin" but at least I have seen non-executables there often. However this might be just the case since I often work with engineers and I for myself did not work in IT (until 4 years ago) and my educational background is far away from IT and Computer Science.

But I have no strong opinion on that I just shared my thoughts in case anybody is interested. So I decide to stay neutral on this topic from now.

rainij commented 2 years ago

@guygastineau I am done with my review. When my suggestions are addressed or at least discussed away^^ I will certainly approve this PR. I hope this saves some time for the real maintainers when they do their review.

guygastineau commented 2 years ago

Geeze, I accidentally started a review, so some of my comments over a few days got bunched up. I just hit submit review to get them to go out. Some of them might have been out of date. Anyway, I think we probably have this ready to go.

rainij commented 2 years ago

@guygastineau I am fine with everything. I approve :) .

Maybe somebody else can give a third opinion on the discussion on bin vs sml-bin vs something else.

kotp commented 2 years ago

Maybe somebody else can give a third opinion on the discussion on bin vs sml-bin vs something else.

bin/ is used to hold executables, so if that is what is in there, the sml track itself is the context of what kind of executables they would be, so sml-bin would be, probably, considered redundant naming.

guygastineau commented 2 years ago

I can change it to bin, but I would like to clarify.

I am not sure that sml-bin is the best name, but I should be clear that redeploy-testlib.sml is not what sml would consider an executable. With mlton we would make an executable like this mlton sml-bin/redeploy-testlib.sml, and this would produce an executable ELF. With PolyML we can also make a standalone executable using polyc sml-bin/redeploy-testlib.sml, although this will fail since the effects are not packed into a function named main. smlnj has several other ways to make executables. With PolyML there is another way to make a special shebang, (which I did first) #! /usr/bin/env -S poly --script, and this lets you execute the "script" directly.

For track tools like redeploy-testlib.sml I see no reason to force maintainers and contributors to compile the code to executable binaries, and the shebang method of turning the code into a script seems a kludge at best, and I don't expect it to be portable for Windows. So, using the Makefile seems like it hits the sweet spot of usability, comprehensibility, and parsimony to me.

I do sometimes have directories called bin in my projects where the contents aren't executable files. In all cases (for me), these are projects using complex build systems like cabal or cargo, and the files in bin are source that will be compiled into executable files that go somewhere else, ie. not back into bin. In this project, as with other exercism projects, the bin directory is more general and typically used for executable files as far as I have experienced. For example, the venerable fetch-configlet is usually found in bin, and after its use we find the static binary configlet appropriate for our local environment in bin as well.

What we are really talking about with sml-bin is a directory with track maintenance tools written in sml such that they can be loaded by an implementation and executed as read. This is very similar to the way that lisp implementations might load a script. Any attempt at calling the files directly will result in an error from your shell, because the source files are not executable. Not only does this approach mean we don't have to worry about shebangs concerning windows compatibility, but using make to drive these tools means we can add means for maintainers to configure the system to use different sml implementations. I am toying with this on the side, and several of my side projects from last year are building/running with poly, smlnj, and mlton. This kind of flexibility is not the purpose of this PR, but I mention this, because the makefile approach will help facilitate this kind of flexibility in the future.


What am I trying to say?

  1. The kind of files in sml-bin are not like the kind of files in bin. bin/generate is executable. bin/fetch-configlet is executable. bin/configlet is executable.
  2. Maybe (likely) sml-bin is not the best name. If so, we should find a better name now before merging the PR. Maybe tools is a better name (or track-tools, etc...).
  3. Lumping these non-executable files, that sml implementations also wouldn't consider executable, into the existing bin directory with clearly executable files is messy and confusing in my opinion. That is the reason for my preference for another directory.

So, if everyone here thinks we should mix the files in bin, then I will just go along with it, but I think there are good reasons for separating them. If we determine, as I suspect, that sml-bin is not the best name, then I think it is worth it for us to figure out a better name for these tools written in sml and used through the makefile.


I hope I have made my rationale clear. I look forward to your reactions.

kotp commented 2 years ago

So, if everyone here thinks we should mix the files in bin, then I will just go along with it, but I think there are good reasons for separating them.

I am against mixing non-executable files in bin/.

The Linux directory structure shows how general directory structure is for an arguably larger project. For example; using usr/ is for some executables and supporting data. So perhaps usr/ is a good place for these things.

ErikSchierboom commented 2 years ago

So, if everyone here thinks we should mix the files in bin, then I will just go along with it, but I think there are good reasons for separating them.

You've made a good argument for separating them.

guygastineau commented 2 years ago

The Linux directory structure shows how general directory structure is for an arguably larger project. For example; using usr/ is for some executables and supporting data. So perhaps usr/ is a good place for these things.

Hmm, I read over the LDS documentation again. I never see files in usr at the top level, rather I see subdirectories like bin, sbin, local, lib, and share. Given how much language in those docs is specific to operating systems I am hesitant to use usr as the home for these files.

Still, I don't think that sml-bin is the best name. I don't think it is catastrophic as a name for the directory, but I would like to do better. Since they are sort of acting like scripts and are getting loaded by an SML implementation, maybe we can call it scripts?

guygastineau commented 2 years ago

By the way, I don't know if you all care enough to debate potential names. Please do if you want to. I see no reason to rush through these contributions, but I would rather not let it languish needlessly either. I will give a couple days for you all to chime in, but provided silence I will go ahead and merge sometime on Tuesday with the current name.

kotp commented 2 years ago

The scripts name might be communicative.

ErikSchierboom commented 2 years ago

Yeah, scripts sounds good!

guygastineau commented 2 years ago

Thank you all for the thorough and thoughtful reviews. We are now using scripts instead of sml-bin which I think is a positive move. I also added a documenting comment to the redeploy script.

@rainij despite an admitted dearth of experience with SML you gave an excellent review. It is a testament to your abilities as an engineer, and I expect the advanced degree in mathematics plays some role in your meticulous contributions to the conversation. Thank you very much.

guygastineau commented 2 years ago

It looks like we do need an SML maintainer or anyone else with write access here to merge the PR.

kotp commented 2 years ago

This was merged a day earlier than I thought was indicated. But looks good @guygastineau . Just had time to do a last check based on the timeline information I had.

guygastineau commented 2 years ago

This was merged a day earlier than I thought was indicated.

Sorry @kotp , I meant I would leave it for a few days to wait for discussion about renaming sml-bin. Given a productive discussion ensued with an acceptable outcome for all participants, I considered the matter settled.

guygastineau commented 2 years ago

By the way, I don't know if you all care enough to debate potential names. Please do if you want to. I see no reason to rush through these contributions, but I would rather not let it languish needlessly either. I will give a couple days for you all to chime in, but provided silence I will go ahead and merge sometime on Tuesday with the current name.

I think this comment of mine is where our miscommunication started, @kotp although rereading your comment stating that scripts might be communicative I realized that I assumed that was a colloquial agreement on the new name, but you might have meant you thought it warranted more discussion. I am sorry if I merged before our discussion was actually over.

kotp commented 2 years ago

It was just that you said "for a couple of days" and so I came back to do a final "after changes" review. It's all good. Would rather have it in, it is an improvement for sure. Just some things I know I have time to come back to, even if I find out that I knew no such thing. ;)