Fully test native Windows in the testsuite#3260
Conversation
Allow for: - Spaces in PATH - Native tools returning \r - Converting paths between Cygwin and Native Signed-off-by: David Allsopp <david.allsopp@metastack.com>
Signed-off-by: David Allsopp <david.allsopp@metastack.com>
Signed-off-by: David Allsopp <david.allsopp@metastack.com>
Replaces simply running the opam command!
|
Travis is expected to fail. Fingers crossed for AppVeyor... (it's passed on my AppVeyor fork) |
|
Thanks a lot!
Maybe it's not done properly everywhere, but locks should almost always be used through the
I am a bit uncomfortable with tweaking the files before validating checksums. I mean, this becomes a weird notion of "file integrity". Wouldn't it be simpler to locally force the value of
err, this is a heuristic at best, we shouldn't base different processing behaviours based on this. It is nowhere specified — and not required — that the On a side-note, there is already an implementation of de-escaping+re-escaping, for the cases where we incrementally expand different variables in interpolated strings (so as to avoid double-deescaping or double expansion)
IIRC there were some cases for which promoting or demoting wasn't supported either on Linux, and we just hope nobody hijacks the lock in the meantime :). Finally, note that a few points above (e.g. the wrapper-based locking mechanism) are new or have been refactored since 1.2, so maybe your work was started before they were there ? Ah, and don't let the nitpicking above reduce the appreciation of all the work involved in this! ¹ which, I agree, may be a design mistake, since it assumes there are no changes between versions at the parsing level. But that's a different matter. |
066aa32 to
1c775df
Compare
|
Will this become a part of 2.0 release or a later release? |
|
There are some technical parts of this (the patching, and so forth) which I think should be in 2.0 but I'd be surprised if @AltGr (and the wider community) is willing to wait for opam 2.0.0 while the Windows support is completed! |
|
Rebased on to master and split 3346-3350. This PR now only contains the final parts to get the testsuite working. |
9eac4b9 to
a134008
Compare
|
So what is exactly a stopper before this get merged? |
4813016 to
e36a32f
Compare
The OpamGit repository backend supports using either Git-for-Windows (or any other Git expecting native-style path naming) or Cygwin Git. Make sure that both are tested. Git's core.autocrlf is also set to the default installation value of true (AppVeyor uses input in its image)
|
Right, the part for building |
This PR contains one commit which is still work-in-progress and all the work is dependent on #3242, #3248 and #3255. It also has to point the testsuite to my unstable fork of opam-repository, so it's definitely not for merging just yet. The squashing of the three base PRs means that GitHub's review interface is largely useless, but this can start the conversations going about the architectural changes.
This PR adds sufficient native Windows support to opam to create a full testsuite pass, and in full VT100 TechniColor. There is still more to come, as it excludes all of the shell integration and additional features required to support Windows well.
The PR is best viewed in two sections: the first 22 commits add/fix the required support; the remaining 14 commits concern porting the testsuite.
(Relatively) Minor changes
Option.map_defaulttoOpamStd.OptionFilenamemodule is now included inOpamCompat(Filename.extensionis required)OpamSystem.resolve_commandwhich caused it to return the directory the resolved command resided in, rather than the command!Filename.check_suffixadded toOpamFilename.Baseand also a functionOpamFilename.Base.add_extensionWindows-related changes
cygpathfor converting between all the various styles (Microsoft have recently introducedwslpathfor WSL). Support for this is added in opam with three low-level functions. OneOpamStd.is_cygwin_variantdetermines whether a given executable itself requires Cygwin, doesn't require Cygwin itself, but is linked to a library which does, or is a native Windows executable. This is used to provideOpamSystem.get_cygpath_functionwhich returns a lazy function for a given command which will convert a supplied path if this is required. Finally,OpamUrl.map_file_urlallows this function to be applied tofile://-style URLs. This support is then used to patch invocations togit,rsync, andtarwhich allows the Cygwin versions of these commands to be used by opam.OpamSystem.release_all_locks. The locking subsystem now keeps a Hashtbl of all lock fds which it knows haven't been released, and this function simply closes them all.OpamRepositoryOpamProcesswhich means that parameters should definitely be passed correctly to Cygwin executables and which also allows scripts to be executed as though Windows supported#!.OpamSystem.classify_executabledetermines whether a given executable file is a script or a binary. This is used to fixOpamSystem.installfor Windows, since it really doesn't make sense to be using Cygwin'sinstallcommand. Various warnings are displayed if inappropriate things get installed (like shell scripts) simply because I couldn't be bothered to split that off into the branch with the shell integration..installfiles so that if an executable filefoois not found, butfoo.exeexists thenfoo.exeis installed.file://URLs, etc..configfiles doesn't work. Tempting as it is to introduce yet another interpolation operator, a simpler scheme is used here: if the first line of the file being interpolated beginsopam-version:, then opam assumes that any interpolation within double-quotes should escape backslashes. It's worked quite nicely in tests - looking at opam-repository, it didn't look as though this was likely to introduce regressions (since anything beginningopam-version:is extremely likely to be a file where having unescaped backslash characters before was a syntax error).LF/CRLF endings
This is a fundamental problem which I think must be addressed before 2.0.0 is released. Git-for-Windows by default sets
core.autocrlfto true, which means that files in git remotes are very likely to get checked out with CRLF line endings, even if they were checked in with LF line endings originally (e.g. opam-repository). This has two key consequences for opam:patchutility, which is surprisingly obtuse about CRLF mismatches. There is a WIP commit in this branch handling the common case for this. The procedure I have come up with here is to pre-process diff files beforepatchis called. One effect of this is to ensure that the patch headers will always use Unix LF line-endings (this should eliminate warnings from patch about stripping trailing CRs). Each file to be patched is then analysed, and the patch updated to either include or strip CR characters based on what is actually on the disk. For example,git diffwill (correctly) produce a patch with Unix line endings for two commits (which have LF normalised endings) even though the files on disk have CRLF checked out line-endings (from core.autocrlf) - sopatchwon't be able to apply the resulting diff file. This seems to be working quite nicely - there's also the possibility (not yet implemented) to work around an inadequacy of some versions ofpatchon Unix which don't correctly implement file renaming. My implementation needsmoresome error handling and the code-paths for context diffs aren't yet implemented, but this is both necessary for the testsuite to pass and, more to the point, working. The intention is that this processing would be always run, since a Unix opam could still be presented with patch files generated on Windows.Testsuite changes
There's a little catalogue of alterations to the testsuite to permit correct operation for AppVeyor:
opam_checkwas linked againstopam-client, but this is a nuisance for mingw as it pullsopam-solverin (which means it needs manifesting on mingw as in Add --with-private-runtime for mingw builds #3255). I've changed it linkopam-stateonly, as that's sufficient.tests/Makefileand the shell scripts in the testsuite both to cope with spaces inPATHand also the stray\rcharacters which come from the output of native Windows executables..acannot be used as a hard-coded extension for library files. I have better solutions in the pipeline (Switch defaults configuration file #2930, etc.) for passing this around, but for nowopam config setis used to create a variableext_libcontaining the correct extension.jbuilder runtestis now run with--no-buffer, so you get to see the results as they happen!tests/Makefilewere a real mess of copy-and-paste with subtle differences, so I factored them out to a macro to ensure that noone will ever understand them again!.config.infiles. I don't think that's relevant to any of the tests (nor even good practice to have a.configwith noopam-versionheader now?)_build/install. The commit mentions that this is done to allow execution ofopam-putenv, but this isn't yet required. It is however required for the mingw builds, since the manifests are only available in the install directory, not the source directory.[mas equivalent to[0m. I like colour, so this is altered both intests/Makefileand also in a couple of places in opam itself. I can't imagine a regression there, since recognising[mas reset and not[0mas reset would be extremely strange.