Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Js_run_binary copy srcs to bin fails on 'hidden' files on Windows #1055

Open
ahmedneilhussain opened this issue May 9, 2023 · 9 comments
Assignees
Labels
bug Something isn't working funding needed Contribute to https://opencollective.com/aspect-build windows Specific to Windows

Comments

@ahmedneilhussain
Copy link

What happened?

Porting some rules_nodejs builds to rules_js, have a source file starting with a dot (.eleventy.js) featuring as input to the srcs attribute of a js_run_binary rule with copy_srcs_to_bin = True.

Falls over with The system cannot find the file specified.

I see the rule generates cmd files wrapping calls to dos copy, which apparently does not copy hidden files (need to use xcopy for that, it seems).

Luckily the file name was not significant for us and renaming it to e.g. coco.eleventy.js fixed the issue.

Version

Development (host) and target OS/architectures:
Windows 10 Pro 21H2
Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz 2.30 GHz (2 processors)

Output of bazel --version:

Version of the Aspect rules, or other relevant rules from your
WORKSPACE or MODULE.bazel file:
https://github.com/aspect-build/rules_js/archive/002397780d51735f1a98b362fca7018df31e85ac.tar.gz
(Need the commit because fix for newer pnpm-lock.yaml syntax not yet in a release)

Language(s) and/or frameworks involved:
bazel 7.0.0-pre.20230316.2

How to reproduce

Create a trivial `js_run_binary` target with a source file starting with a period and copy_srcs_to_bin = True, and try to build the target on windows.

Any other information?

No response

@ahmedneilhussain ahmedneilhussain added the bug Something isn't working label May 9, 2023
@github-actions github-actions bot added the untriaged Requires traige label May 9, 2023
@gregmagolan gregmagolan removed the untriaged Requires traige label May 9, 2023
@gregmagolan gregmagolan moved this to 🔖 Ready Todo in Open Source May 9, 2023
@gregmagolan
Copy link
Member

Thanks for the bug report and analysis. Presumably, changing the bazel-lib to xcopy here would fix this? https://github.com/aspect-build/bazel-lib/blob/d13884f29d4f6cfc1ae4dd9d9ac9334ac0de9214/lib/private/copy_file.bzl#L45

@gregmagolan
Copy link
Member

cc @jbedard

@jbedard
Copy link
Member

jbedard commented May 10, 2023

@ahmedneilhussain would you be able to test the fix from bazel-contrib/bazel-lib#427?

@jbedard jbedard moved this from On Deck to 👀 In review in Open Source May 10, 2023
@ahmedneilhussain
Copy link
Author

@ahmedneilhussain would you be able to test the fix from aspect-build/bazel-lib#427?

Sure - I have just rebuilt my windows environment for a different part of the stuff I was working on when I ran into this, so it might take a short while until I can try it - if I don't follow up tomorrow please nag me.

Since I'm already invoking rules_js off a commit rather than a release I guess I can just point the WORKSPACE url at your GitHub fork & branch and it should just work?

Was there no need to use the /H flag on xcopy? I needed to do that when I tried it manually. Unfortunately the windows command line tools are really primitive and have very irregular syntax so I'm not able to confirm that xcopy can just be dropped in as a replacement for copy in all cases, as I'm definitely no expert in this space...

@jbedard
Copy link
Member

jbedard commented May 11, 2023

Since I'm already invoking rules_js off a commit rather than a release I guess I can just point the WORKSPACE url at your GitHub fork & branch and it should just work?

Correct, I think just replacing the bazel-lib url with the commit in my PR should work. With the latest commit in that PR it would be: https://github.com/aspect-build/bazel-lib/archive/89de30b47d169ff10025ea0955867db7aa21ec68.zip

@ahmedneilhussain
Copy link
Author

Hi, I've tried it and I'm afraid it doesn't work. It hangs waiting for input. See https://stackoverflow.com/questions/4283312/why-does-the-command-xcopy-in-batch-file-ask-for-file-or-folder for the grim details. I think your best bet is to use the target directory rather than the target file as a second argument. I also think you need the /H flag. See https://learn.microsoft.com/en-us/troubleshoot/windows-client/deployment/switches-with-xcopy-and-xcopy32-command

@jbedard
Copy link
Member

jbedard commented May 12, 2023

@ahmedneilhussain do you think you'd be able to checkout the branch in bazel-contrib/bazel-lib#427 and try your suggestion? Otherwise looks like I'll have to find a windows machine...

@ahmedneilhussain
Copy link
Author

Hi, I played around with it but unfortunately xcopy A B is problematic when A and B are both single files - xcopy tries to be too clever and asks whether you want to copy to a file called B or copy A into a directory called B. There is no command line option to suppress this, unbelievably, and the hack proposed in the stack overflow answer I linked above (namely, piping a single character answer to the question into the command) is locale-specific.

I tried replacing dst.path with dst.dirname in copy.bzl but that won't work when you are copying and renaming a file at the same time.

I suppose one could try (x)copying the file to a temporary name and renaming it when in place, but that's obviously extra logic. I also haven't checked to see whether DOS rename plays nicely with hidden files. Windows cmd line utilities are horrible 1970s relics so trying to do anything industrial strength and reliable with them is an uphill struggle. Microsoft isn't really bothering with them since powershell came along but that isn't guaranteed to be available.

We use python binaries to wrap the various much more robust python file handling utilities in our bazel builds, but I suppose that's a huge dependency to impose on your consumers...

@jbedard
Copy link
Member

jbedard commented May 16, 2023

I don't think we'll spend much more time on this, windows isn't our priority unless we get funding for it.

@ahmedneilhussain feel free to open a PR if you have any other ideas or solutions

@jbedard jbedard added the funding needed Contribute to https://opencollective.com/aspect-build label May 16, 2023
jbedard added a commit to bazel-contrib/bazel-lib that referenced this issue May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working funding needed Contribute to https://opencollective.com/aspect-build windows Specific to Windows
Projects
No open projects
Status: 👀 In review
Development

Successfully merging a pull request may close this issue.

3 participants