Fix/disable tests on cygwin #14576
Fix/disable tests on cygwin #14576
Conversation
Currently, --gtest_filter=nix_api_store_test.nix_eval_state_lookup_path will result in: terminating due to unexpected unrecoverable internal error: Assertion 'gcInitialised' failed in void nix::assertGCInitialized() at ../src/libexpr/eval-gc.cc:138 Changing the test fixture to _exr_test causes GC to be initialised.
src/libexpr-tests/primops.cc
Outdated
| CASE(R"({ outPath = "foo"; })", "foo"), | ||
| CASE(R"(./test)", "/test"))); | ||
| CASE(R"(./test)", | ||
| // canonPath("//./test", false) on cygwin returns //./test |
There was a problem hiding this comment.
This is messy, but I'm not really sure where to fix this.
// For Windows
auto rootName = std::filesystem::path{path}.root_name();
This ends up being "//." on cygwin, but on linux it's "", even though the path passed is "//./test" in both cases.
There was a problem hiding this comment.
What does //./test mean on Cygwin? That looks to me more like /test than ./test?
There was a problem hiding this comment.
"//./test" is what ends up getting passed into canonPath on both linux and cygwin from what I can tell. The difference is that root_name returns an empty string on linux, so:
if (!rootName.empty())
ret = rootName.string() + std::move(ret);
puts the "//." back on the front, but only on cygwin.
I don't have the C++ spec, and the public docs are not really clear about how root_name should work. So I'm not really sure if this is a bug in the C++ implementation or a bad assumption by nix.
There was a problem hiding this comment.
Also, I don't think we want to be using std::filesystem stuff with CanonPath / nix language paths at all anyways.
So even in those grounds this should be fixed.
There was a problem hiding this comment.
commit 319ec6f84accb7342160b856185402dcdebbaba9
Author: John Ericson <John.Ericson@Obsidian.Systems>
Date: Sun Jan 14 14:30:25 2024 -0500
Support Windows paths in `canonPath` and `absPath`
`canonPath` and `absPath` work on native paths, and so should switch
between supporting Unix paths and Windows paths accordingly.
The templating is because `CanonPath`, which shares the implementation,
should always be Unix style. It is the pure "nix-native" path type for
virtual file operations --- it is part of Nix's "business logic", and
should not vary with the host OS accordingly.
I assume that's what you meant in the last paragraph there, but the problem here is with canonPath.
There was a problem hiding this comment.
To be clear the "this should be fixed" should not block this PR. We just need a comment saying this is a temp hack and the issue should be fixed.
I meant CanonPath. canonPath and CanonPath should not be used together. nix language paths are implemented with Source path which uses CanonPath. And so we shouldn't use canonPath in the interpreter like we appear to be doing so here.
There was a problem hiding this comment.
Yeah let's just skip this case with CygWin, rather than testing for the wrong answer, I think.
There was a problem hiding this comment.
That makes sense. I updated it with an attempt at a FIXME comment, and just skipping the case. I'm not 100% sure the comment captures what you're suggesting, so let me know if you'd like me to change it.
There was a problem hiding this comment.
Thanks! I think it does capture it, or at least is good enough for now.
corngood
force-pushed
the
cygwin-tests
branch
from
4862025 to
b78c7f1
Compare
| @@ -1,4 +1,5 @@ | |||
| #ifndef _WIN32 | |||
| // TODO: investigate why this is hanging on cygwin | |||
| #if !defined(_WIN32) && !defined(__CYGWIN__) | |||
There was a problem hiding this comment.
This is not ideal, but I think it's more likely to be a bug in cygwin than nix.
corngood
force-pushed
the
cygwin-tests
branch
from
b78c7f1 to
b115c90
Compare
2 participants
This is enough to get through all the tests that are run while building the modular nix from nixpkgs on cygwin.