Eliminate cp scripts in ocamltest

[dra27]

or "Allow the test directory to be set-up declaratively". Tipping context for this is #2023 (comment), where the fact on Unix ocamltest symlinks files mentioned in the files declaration but on Windows it copies them was causing tests to fail, since the intention is that the files were then modified by the tests in question (this didn't just break ocamltest hygiene, it also broke the test)

It would be nice to be able to specify files which must be copied to the test directory and in so doing eliminate mkdir and cp script actions. The other motivation is that this header:

(* TEST
files = "cant_write_to_this_file.txt"
*)

has to become the more verbose:

(* TEST
* script
script = "cp ${test_source_directory}/cant_write_to_this_file.txt can_write_to_this_file.txt"
** byte
** native
*)

My understanding is that if the byte and native actions are omitted, then the test becomes the cp and will always pass? For this case, I propose:

(* TEST
data_files = "can_write_to_this_file.txt"
*)

but a thorough solution to this issue should aim to eliminate all script actions related to setting up the test build directory.

[shindere]

I confirm that if a test or action is mentionned, then this overrides
the tests performed by default, namely bytecode and native.

Thanks a lot for the proposal which I find indeed interesting!

[Anukriti12]

Hi, I am an outreachy applicant. Can i work on this issue?

[shindere]

Givin it a second thought: wouldn't it be more straightforward to copy files systematically rather than creating symlinks on Unix? It feels to me this would solve the described problems, wouldn't it?

[dra27]

Given the sheer number of files in a testsuite run, I would expect that it's noticeably slower to copy (longer term, I'd hope to go the other way and have Windows do symlinking, given that most developers will be at Windows boxes which support it).

Switching between cp and ln also doesn't solve the problem for tests with more complicated set-ups like tests/regression/missing_set_of_closures/missing_set_of_closures.ml (and a lot of the Dynlink tests).

[gasche]

@Anukriti12 Given the uncertainty / non-consensus on how to solve the problem described in this thread, I would avoid it for now.

Others: personally I agree with the idea of having an easy way to specify files to import in the test directory. I also think that files imported there should be non-writeable by default, unless explicitly designed to be written to. I am not sure what's the best way to let test-authors express the intent that a file will be written to. (I don't understand from your proposal if data_files vs. files is supposed to express this difference; unconvinced.)

[github-actions]

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

[gasche]

@dra27, @shindere, does any of you have opinions on how to move forward on this issue? If not, or if it's not something you want to look at in the medium term, maybe we could close?

[shindere]

So if we have two variables, one meaning hte files it contains can be symlined, the other saying that the listed files must be copied, that will solve theproblem, right? If we all agree on this, then perhapsthe only thing that needs to be decided is what the names ofthe variables should be and then we can implemnet the solution and move on?

[dra27]

I still think this is a good idea, but I'm not going to be working on it soon, indeed. We have an issue that the idea of longer-term "we agree this would be a good idea to do this at some point" and the "close stale issues bot"/"don't leave PRs lying around" are in conflict! The original test bitten by this issue actually needed to work around it in a different (to deal with line endings), but the principal idea of eliminating scripts (and sandboxing tests) remains useful both for testing installed compilers (which we are working on for the flambda 2 work) and cross-compilation (which we're not working on at present).

An option (which I'm personally doing with some of my "back-burner" lists privately) might be to have ROADMAP.md in addition to HACKING.md? Discussions like this could lead to an update to the ROADMAP.md file. It persists the discussion where it can be picked up in future, and it would be relatively easy to review them from time to time (either to remove items which have been done, or to remove ones which for whatever reason no longer make sense).

[gasche]

re. issue management: personally I think that a "suspended" tag would be fine (and we could leave the issue open in this status), if we use it with care. Sounds easier than having to decide how to structure a ROADMAP file.

Regarding the naming itself: if I understand, currently files means immutable files (that can be symlinked.) I would suggest maybe local_files or copied_files for the ones that may be modified by the test, but whose modifications should not be persisted in the testsuite source.

[shindere]

@dra27: can we perhaps remian close to the topic itself and not going too high? I'm willing to implemnet the solution if we can agree on what it should be and if it's simple. Do you like the two variables approach, or not? If yes, can we agree on the names?

[shindere]

Thanks, @gasche. We could also rename `files` to something else and less ambiguous, e.g. `symlinked_files`, so we would have this and `copied_files`. As I said, I wouldn't mind doing the job once we agreed on what it should be.

[dra27]

@dra27: can we perhaps remian close to the topic itself and not going too high? I'm willing to implemnet the solution if we can agree on what it should be and if it's simple. Do you like the two variables approach, or not? If yes, can we agree on the names?

Ah, lovely! In which case yes, no problem. My only reason for being slightly higher level about it was if we were to further the discussion but not implement it, then we'd just have the bot trying to close the issue again in 6 months! It wasn't clear that discussing it would make progress, if you see what I mean...

I agree that renaming files to eliminate the surprise is a good idea, but I don't think it should refer to the mechanism (especially as we don't actually symlink the files on Windows at the moment).

Looking at the uses of files in the tests, could we change files to source_files or sources and use @gasche's suggestion of local_files for data? local_files could be used, say, for tests/tool-lexyacc/calc_input.txt (even though it's not actually altered).

[shindere]

David Allsopp (2021/03/24 04:03 -0700):

> @dra27: can we perhaps remian close to the topic itself and not going too high? I'm willing to implemnet the solution if we can agree on what it should be and if it's simple. Do you like the two variables approach, or not? If yes, can we agree on the names? Ah, lovely! In which case yes, no problem. My only reason for being slightly higher level about it was if we were to further the discussion but not implement it, then we'd just have the bot trying to close the issue again in 6 months! It wasn't clear that discussing it would make progress, if you see what I mean...

I see. I think I already mentionned that I was willing to move on with this but perhaps I was not clear enough. Regarding the names of the variables, how about `read_files` and `written_files`, or `modified_files` for the second one?

[dra27]

For some reason I read the "read" in read_files in the present tense (!!) and got briefly confused. Which is embarrassing when I'm in my first language and you're in your second, but there we are!

Perhaps readonly_files (which clearly states the possibility in the future of enforcing it either by file permissions or sandbox) and modifiable_files (I definitely prefer modified to written)?

[shindere]

OK, `readonly_files` and `modifiable_files` looks good to me. Shall I work in the current PR and push to your branch, or shall we close the presentPR and create a new one that owuld refer to the present one? I guess the second aproach looks simpler to me but I'll let the final decision be yours, @dra27, and, if you agree wiht me creating a different PR, I'll let you close this one, if you don't mind.

[dra27]

@shindere - there's no branch for this, it's just an issue

[shindere]

David Allsopp (2021/03/24 14:22 +0000):

@shindere - there's no branch for this, it's just an issue

My bad, sorry! Let's keep it open then an we will close it when the correspponding PR gets merged. Sorry for my lack of attention!

[shindere]

@dra27 and possibly others: looking at e.g. `testsuite/tests/lib-dynlink-native/main.ml`, the test uses `cp` to copy four files from the `sub` directory located in the test's source build directory to the very same `sub` directory in the test build directory. That use of `cp` looks kind of wrong to me. Shall we just create a `sub` symlink in the test's build directory? And how shall that be described in ocamltest? Would it be okay, for instance, that `readonly_files` is allowed to also contain directories, whicl would also be symlinked as the files are currently? The `files` bit would then seem a bit misleading to me but I can't think of anything else so if it seems okay to others to do like this I'll do so.

[shindere]

If a directory is added to the `files/`/`readonly_files` variable, the current code already "works", meaning that it creates a symlink in the build directory to the source subdirectory. That, however, does not quite bring the desired effect. First, object files may be created in the source directory, which should be avoided. Second, the test on which I used that does not work, at some point the compiler does not find a module it is looking for. I don't intend to investigate this, because even if the test would pass, the behavour of adding files to the source subdirectory is not the intended one anyway. So one possible approach is to detect if a so-called "file" actually is a directory and actually recursively copy it from source to build tree. Will try this now. Of course directories to copy could be put in a separate variable, but that's a detail which is not important here, I think. The important point here, as I understand it, is that directories get copied and that this should be possible to do in a declarative way from the ocamltest DSL.

[dra27]

I agree, @shindere - directories definitely don't want to be symlinked, but "copied" (in the readonly case, the files could of course themselves be symlinked).

[shindere]

David Allsopp (2021/04/01 04:11 -0700):

I agree, @shindere - directories definitely don't want to be symlinked, but "copied" (in the readonly case, the files could of course themselves be symlinked).

Well at the moment I am working on a solution where I have created a `subdirectories` variable which lets one copy subdirectories recursively. I am now re-writing the tests with this variable and see when this is done what will remain, in which ones this new variable does not allow to eliminate cp.

[shindere]

Sébastien Hinderer (2021/03/24 14:37 +0100):

OK, `readonly_files` and `modifiable_files` looks good to me.

So, I just submitted #10327 I couldn't find any test where `modifiable_files` would have been handy, though. So I introduced the ocamltest `subdirectories` variable, to declare subdirectories to be recursively copied from the test source tree to its build tree.

[shindere]

As stated in
#10327 (comment), it is my
belief that PR #10327 fully addresses this issue, which I am thus closing.

Anybody should however feel free to re-open it if they think it's not
completely fixed.