TSCBasic: repair the Windows build after #277 #282

compnerd · 2022-01-13T19:17:49Z

This fixes multiple issues with the file locking implementation on
Windows. AbsolutePath.root is meaningless on Windows - there is
no concept of a singular root on Windows, instead you have have 26
individual roots - A-Z. Stripping the count of characters for the
root path leaves us with an invalid path string as the root drives
have been stripped. The next character is an invalid character on
most file systems, and so we must replace :. Instead of using a
MAX_NAME to compute a length of the ARC, enforce a limit for the
complete path (though an ARC would be limited to 255 codepoints on
NTFS). This incidentally also corrects the emitted path, previous
path computation left us with the file in PWD.

compnerd · 2022-01-13T19:17:55Z

@swift-ci please test

compnerd · 2022-01-13T19:22:36Z

@abertelrud I think that this is mostly similar to #281 but missing the changes you found on the test suite, but it has the additional file name generation fix. I think that we need part of your change still, a part of this change, and a part of another change to get this fully correct.

abertelrud · 2022-01-13T19:24:54Z

@abertelrud I think that this is mostly similar to #281 but missing the changes you found on the test suite, but it has the additional file name generation fix. I think that we need part of your change still, a part of this change, and a part of another change to get this fully correct.

Thanks for this! Right, at least the unit test's use of NAME_MAX needs to be conditioned out (I updated that PR while you were putting up this one).

compnerd · 2022-01-13T19:26:19Z

I should be more explicit: I don't know how to do this M&A exercise - any thoughts on how to accomplish this the most effective way?

abertelrud · 2022-01-13T19:26:50Z

I am not sure about the origins of the original AbsolutePath.root but it was there already. So I guess there's a silver lining here in that there are some non-Windows-compatible runtime issues being addressed.

abertelrud · 2022-01-13T19:27:42Z

I should be more explicit: I don't know how to do this M&A exercise - any thoughts on how to accomplish this the most effective way?

I think we should proceed with this PR but just put in the conditional from #281 to skip that part of the unit test.

compnerd · 2022-01-13T19:28:53Z

Once the dust from these changes settles, I believe that #278 also needs to be updated.

abertelrud · 2022-01-13T19:29:46Z

Once the dust from these changes settles, I believe that #278 also needs to be updated.

Yes, absolutely.

abertelrud · 2022-01-13T19:30:22Z

Sources/TSCBasic/Lock.swift

+                                .components.joined(separator: "_")
+                                .replacingOccurrences(of: ":", with: "_") + ".lock"
+#if os(Windows)
+        var lockFileUTF8 = lockFilesDirectory.appending(component: lockFileName).pathString.utf8.suffix(Int(MAX_PATH))


This is supposed to be just the filename, so lockFilesDirectory gets prepended again below.

This means you'll end up with lockFilesDirectory twice.

Looking at this more, I think the part about AbsolutePath.root was try avoid pasting something that starts with / as a subpath. I kept that part unmodified but it seems odd, since the file name should never have that in the first place.

My view is that we should just compute a SHA1 of the whole path, whatever it is, and then use that as the filename. But it's true that it would be less readable.

Oh I see: With non-Windows file paths, the components returns a / as the first path component. So this is just stripping that out.

Taking a step back, I think what would make sense here would be to take just the components leading from the root of the path (whether that's / or C:\ or whatever) and just using those as the basis for the filename. All this is trying to do is to come up with a unique file name.

Is there an API to return just the path and not the root?

That would be better, however, I think that this new version should address the path issue.

Is there an API to return just the path and not the root?

I don't believe that there is such an API in the TSC Path API today.

Sources/TSCBasic/Lock.swift

This fixes multiple issues with the file locking implementation on Windows. `AbsolutePath.root` is meaningless on Windows - there is no concept of a singular root on Windows, instead you have have 26 individual roots - A-Z. Stripping the count of characters for the root path leaves us with an invalid path string as the root drives have been stripped. The next character is an invalid character on most file systems, and so we must replace `:`. Instead of using a `MAX_NAME` to compute a length of the ARC, enforce a limit for the complete path (though an ARC would be limited to 255 codepoints on NTFS). This incidentally also corrects the emitted path, previous path computation left us with the file in PWD.

abertelrud · 2022-01-13T19:48:14Z

These new changes look good to me. The only thing left is to conditionalize the unit test which references NAME_MAX (but doesn't really need to — it could just be 255).

abertelrud · 2022-01-13T19:48:31Z

Then any kind of exceeding of PATH_MAX could be done in a separate PR.

compnerd · 2022-01-13T19:48:54Z

@swift-ci please test

abertelrud · 2022-01-13T19:49:35Z

Thanks for helping to deal with this and sorry it landed on you unexpected. I didn't realize this would have a Windows effect, and I never was able to get Swift up and running in a Windows VM (it's not really feasible to manually test all changes in a separate Windows VM locally anyway)

compnerd · 2022-01-13T19:50:04Z

I'd like to keep the authorship information for the changes to the test case (you noticed that and fixed it) which is why I haven't folded that into this change.

abertelrud · 2022-01-13T19:53:14Z

I'd like to keep the authorship information for the changes to the test case (you noticed that and fixed it) which is why I haven't folded that into this change.

We need that, though, I think for this to pass? I certainly don't mind folding it in here, unless you specifically want to disavow it. :) But the fix is actually different — now that the Windows side is fixed too, through your changes, I think it makes sense to not conditionalize the unit test but just put the constant 255 instead of NAME_MAX there. Since it's just constructing test strings by repeating a phrase.

abertelrud · 2022-01-13T20:01:42Z

I'd like to keep the authorship information for the changes to the test case (you noticed that and fixed it) which is why I haven't folded that into this change.

We need that, though, I think for this to pass? I certainly don't mind folding it in here, unless you specifically want to disavow it. :) But the fix is actually different — now that the Windows side is fixed too, through your changes, I think it makes sense to not conditionalize the unit test but just put the constant 255 instead of NAME_MAX there. Since it's just constructing test strings by repeating a phrase.

So what do you suggest as the next step here? I think the only thing missing in this PR is NAME_MAX -> 255 in the unit test.

After that I'll update #278

compnerd · 2022-01-13T20:30:12Z

I don't mind at all if you push your bits of the changes to this change at all, I just care about the authorship information :)

abertelrud · 2022-01-13T20:35:38Z

I don't mind at all if you push your bits of the changes to this change at all, I just care about the authorship information :)

Ok, I see, so to check out your branch and add a commit to it? Happy to do that. I think the current CI will fail if we don't, right? This PR still doesn't build on Windows IIUC?

compnerd · 2022-01-13T20:36:27Z

I've built it on Windows locally, and been able to use the generated TSCBasic.dll to run swift-build, so I expect that this should be okay.

abertelrud · 2022-01-13T20:38:14Z

I've built it on Windows locally, and been able to use the generated TSCBasic.dll to run swift-build, so I expect that this should be okay.

Ok, sounds good. Did you build the unit tests on Windows too? Because I don't see how those would compile.

compnerd · 2022-01-13T20:39:16Z

Ok, sounds good. Did you build the unit tests on Windows too? Because I don't see how those would compile.

I did not, which is why I never noticed that.

abertelrud · 2022-01-13T20:42:03Z

Ok, I added a commit to your branch in your fork, so the attribution information should be correct.

abertelrud · 2022-01-13T20:42:10Z

@swift-ci please test

compnerd · 2022-01-13T20:42:17Z

Thanks @abertelrud!

abertelrud · 2022-01-13T20:43:46Z

Thanks @compnerd for your fixes here, and sorry about now having to restart the tests. Meanwhile we should get @tomerd's signoff, and then I will update #278.

compnerd requested review from abertelrud, aciidgh, friedbunny, neonichu and tomerd as code owners January 13, 2022 19:17

compnerd mentioned this pull request Jan 13, 2022

Revert "FileLock encounters a runtime error when the path of the locked file is long" #280

Closed

abertelrud reviewed Jan 13, 2022

View reviewed changes

compnerd force-pushed the 277 branch from 0571626 to 9ef1f15 Compare January 13, 2022 19:39

abertelrud reviewed Jan 13, 2022

View reviewed changes

Sources/TSCBasic/Lock.swift Outdated Show resolved Hide resolved

compnerd force-pushed the 277 branch from 9ef1f15 to 05313f9 Compare January 13, 2022 19:44

compnerd force-pushed the 277 branch from 05313f9 to 81d45cf Compare January 13, 2022 19:46

Avoid using NAME_MAX in unit tests since it is not available on Windows.

27ebddb

abertelrud mentioned this pull request Jan 13, 2022

Fix a build failure on Windows, where NAME_MAX doesn't exist #281

Closed

tomerd approved these changes Jan 13, 2022

View reviewed changes

abertelrud mentioned this pull request Jan 13, 2022

[5.6] FileLock encounters a runtime error when the path of the locked file is long (#277) #278

Merged

compnerd merged commit 4afd18e into swiftlang:main Jan 14, 2022

compnerd deleted the 277 branch January 14, 2022 03:38

TSCBasic: repair the Windows build after #277 #282

TSCBasic: repair the Windows build after #277 #282

Uh oh!

Conversation

compnerd commented Jan 13, 2022

Uh oh!

compnerd commented Jan 13, 2022

Uh oh!

compnerd commented Jan 13, 2022

Uh oh!

abertelrud commented Jan 13, 2022

Uh oh!

compnerd commented Jan 13, 2022

Uh oh!

abertelrud commented Jan 13, 2022

Uh oh!

abertelrud commented Jan 13, 2022

Uh oh!

compnerd commented Jan 13, 2022

Uh oh!

abertelrud commented Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abertelrud Jan 13, 2022

Choose a reason for hiding this comment

Uh oh!

abertelrud Jan 13, 2022

Choose a reason for hiding this comment

Uh oh!

abertelrud Jan 13, 2022

Choose a reason for hiding this comment

Uh oh!

abertelrud Jan 13, 2022

Choose a reason for hiding this comment

Uh oh!

compnerd Jan 13, 2022

Choose a reason for hiding this comment

Uh oh!

compnerd Jan 13, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

abertelrud commented Jan 13, 2022

Uh oh!

abertelrud commented Jan 13, 2022

Uh oh!

compnerd commented Jan 13, 2022

Uh oh!

abertelrud commented Jan 13, 2022

Uh oh!

compnerd commented Jan 13, 2022

Uh oh!

abertelrud commented Jan 13, 2022

Uh oh!

abertelrud commented Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

compnerd commented Jan 13, 2022

Uh oh!

abertelrud commented Jan 13, 2022

Uh oh!

compnerd commented Jan 13, 2022

Uh oh!

abertelrud commented Jan 13, 2022

Uh oh!

compnerd commented Jan 13, 2022

Uh oh!

abertelrud commented Jan 13, 2022

Uh oh!

abertelrud commented Jan 13, 2022

Uh oh!

compnerd commented Jan 13, 2022

Uh oh!

abertelrud commented Jan 13, 2022

Uh oh!

Uh oh!

abertelrud commented Jan 13, 2022 •

edited

Loading

abertelrud commented Jan 13, 2022 •

edited

Loading