Closed Bug 1647582 Opened 6 years ago Closed 5 years ago

`mach vendor rust` has problems on Windows

Categories

(Firefox Build System :: General, defect, P3)

Product:
Firefox Build System
Component:
General
Type:
defect

Tracking

(firefox82 fixed)

Status:
RESOLVED FIXED
Milestone:
82 Branch
Tracking Flags:
Tracking Status
firefox82 --- fixed

People

(Reporter: n.nethercote, Assigned: Gankra)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

Bug 1647582 - add new version checks to mach vendor rust. r?njn
47 bytes, text/x-phabricator-request
Details | Review

Bug 1619788 was a problem, but it just got fixed.

Also, symlinks aren't handled correctly. There's a Cargo issue on file. This seems to be at root a minGW/Windows filesystem issue, though it might be possible for Cargo to work around it?

Note: once mach vendor rust works fully on Windows, we should remove the note in build/docs/rust.rst about it not working fully on Windows.

Blocks: rust-great

Not triaging this since according to comment 0 this appears to just be a cargo bug.

Severity: -- → S3
Priority: -- → P3

If I run ./mach vendor rust on an unchanged m-c, on Linux and Mac I get no changes. But on Windows I get huge numbers of changes:

 966 files changed, 281660 insertions(+), 281919 deletions(-)

But most of those changes are just changing line endings of files within third_party/rust/. Ricky, do you know how to fix that?

Flags: needinfo?(rstewart)

This smells like cargo's libgit2 doing crlf conversions it shouldn't be doing. Which could be because of the global git configuration.

@njn confirmed on Matrix that this is related to core.autocrlf being true on his machine. It's the kinda default when you install Git for Windows, but you can actually choose something else in the installer. What this also means is that this should be reproducible on other OSes by setting core.eol to crlf. It seems to me cargo should preserve whatever end-of-lines are in the original crates.

glandium suggested:

  • Run git config --global core.autocrlf false
  • Delete ~/.cargo/git and ~/.cargo/registry`
  • Re-run ./mach vendor rust

That gave much better results. The line endings problem went away. Six dot files were erroneously modified, though:

  • third_party/rust/lucet-wasi/examples/.gitignore was deleted
  • third_party/rust/lucet-wasi/.cargo-checksum.json was modified to account for the above deletion
  • third_party/rust/spirv-cross-internal/src/vendor/SPIRV-Cross/.clang-format was deleted
  • third_party/rust/spirv-cross-internal/src/vendor/SPIRV-Cross/.gitignore was deleted
  • third_party/rust/spirv-cross-internal/src/vendor/SPIRV-Cross/.travis.yml was deleted
  • third_party/rust/spirv-cross-internal/.cargo-checksum.json was modified to account for the above deletions.

The Cargo in my PATH is 1.44.1.

So, two questions:

  • Is the line endings thing affecting other people?
  • How to fix the dot file problems?

gankra: are you interested in looking into this? I'd start just by running ./mach vendor rust on an unchanged m-c repository and see what happens, whether you get the line-ending weirdness that I did. Beyond that, for the dot file issues, I guess you'd need to work out exactly what cargo commands are being run by ./mach vendor rust, and see if they're working as they should. Bug 1644371 also dealt with dot file issues, and may be relevant.

Flags: needinfo?(a.beingessner)
Flags: needinfo?(rstewart)

I cleaned off the dust on my windows dev machine, updated the OS, pulled central (538532:f0ac79e1ed53), ran rustup, reconfigured, rebuilt, and then ran mach vendor rust.

It ran for a bit and at the end, hg diff shows no changes to my project. So, at least on my laptop, everything seems to be working perfectly? My copy of mozillabuild is a bit old, dating back to May 3, 2018 (VERSION 3.2). Maybe this is a more recent regression? There's only one release after it (3.3), so idk. Could also have been fixed upstream in cargo or something?

Flags: needinfo?(a.beingessner)

Updating mozbuild sounds wise, can you try that?

Also, what is the output of git config core.autocrlf on your machine?

Finally, you could try making a change (e.g. add a fake dependency to a crate, e.g. add a dependency on bitflags 1.0 to modules/libpref/parser/Cargo.toml) and see if it does the same thing as a Linux or Mac machine.

Flags: needinfo?(a.beingessner)

Still not getting anything bizarre on my windows machine.

Things done:

  • Updated to mozillabuild 3.3 (following the instructions on its wiki page)
  • Reran mach bootstrap stuff / clobbered
  • git config core.autocrlf was false (possible I once set this in the past 3 years)
    • note: I'm not using cinnibar for gecko on windows, although of course git is still involved for cargo
  • Added a bitflags 1.0.0 dep to libpref/parser
  • Added the same dep on macos
  • ran mach vendor rust

result: both machines produced the same trivial/clean changes

possibly you should run a vendor yourself to see if things were magically fixed in the last few weeks?

Flags: needinfo?(a.beingessner) → needinfo?(n.nethercote)

I'm still getting the changes to the six dot files mentioned in comment 12. Hmm.

Flags: needinfo?(n.nethercote)

Update: I built a new work machine, and my basically pristine new install of windows + mozillabuild reproduces njn's issues. Investigating now.

Note: the windows git installer prompts you to set core.autocrlf, but the default is true (as noted above, we prefer false).

ok so the non-autocrlf issue is a bug in cargo vendor which was fixed in Rust 1.45, the latest stable release as of 3 days ago.

So maybe we add a version check to cargo vendor and the is fixed?

Ok final summary of this bug. It is in fact two bugs:

  1. cargo vendor behaves poorly when git config core.autocrlf=true, needlessly rewriting the vendored library's newlines
  2. cargo vendor in versions of rust earlier than 1.45 (the latest stable) will deterministically but nonsensically delete and modify some files when run on windows (msys?) (for historical record, you can reproduce this on gecko commit 540166:426eecf976c0).

Issue 2 ends up being fairly straightforward: at most we just need to add a version check to mach vendor rust. We could make it conditional on windows, but honestly we might as well make it universal for simplicity, since it's the latest stable release, and vendor is an ~advanced developer feature.

That said, it would be nice to know why it was fixed in 1.45. Nothing in the cargo release notes suggests an answer.

Issue 1 is more subtle. Some observations:

  • If you have not installed git, cargo's libgit2 will default to autocrlf=false (good!) (presumably it's the absence of standard git config files more than anything)
  • If you do install git with the official wizard, it will prompt you for the value you want, but defaults to autocrlf=true (bad!)

Glandium asserts that true is actually a good value for end user development on windows (no opinion), and therefore believes we should regard this as a bug in cargo/libgit2/mach-vendor-rust to set/ignore the value properly.

Alternatively, we can regard this as just a "bad configuration" for firefox development, and teach mach vcs-setup and/or mach bootstrap to prompt you to reconfigure it.

Assignee: nobody → a.beingessner

Glandium asserts that true is actually a good value for end user development on windows

In general. Not too sure about Firefox specifically. But there are two problems with tweaking the git configuration:

  • we can't change the config for cargo vendor only, any change will affect all uses of git on the machine.
  • mach vcs-setup/mach bootstrap tweak the configuration of the tool used to clone and update the Firefox tree, but in the case of cargo vendor, the problem is always with the git configuration, whether Firefox is cloned from git or mercurial.

+1 to autocrlf=true being a good thing that we don't want to start forbidding or discouraging. It's especially useful for cross-platform, heavily distributed projects where Windows development occurs -- i.e., exactly what Firefox is.

Really it's entirely possible to have a perfectly sensible Git workflow on Windows with either value of autocrlf. cargo vendor should be aware of this and support any vaue autocrlf might have, either by default or via a command-line option that we can use.

Could mach vendor temporarily set autocrlf to false and then set it back if necessary?

I have submitted a fix for issue 1 upstream: https://github.com/rust-lang/cargo/pull/8523

The upstream fix has landed, so if you run a recent rust/cargo nightly, all of the issues reported here should be fixed. So the only remaining question is if we wish to put a check in to cargo vendor that you're running a recently nightly (1.47.0, 2020-07-26 should suffice), or if it's just a documentation issue?

(In reply to Alexis Beingessner [:Gankra] from comment #20)

The upstream fix has landed, so if you run a recent rust/cargo nightly, all of the issues reported here should be fixed. So the only remaining question is if we wish to put a check in to cargo vendor that you're running a recently nightly (1.47.0, 2020-07-26 should suffice), or if it's just a documentation issue?

I am not wild about the idea of requiring nightly rust/cargo to effectively develop Firefox, but maybe if the check is Windows-only, and we made it so mach vendor would transparently use the nightly version, that would be OK?

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:Gankra, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(a.beingessner)
Pushed by abeingessner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/154ec7ab29d0 add new version checks to mach vendor rust. r=njn

https://hg.mozilla.org/mozilla-central/rev/154ec7ab29d0

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox82: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch
Flags: needinfo?(a.beingessner)
Blocks: 1938341

We encountered another mach vendor rust issue on Windows in CI.

When vendoring neqo crates into Gecko, the add_remove_files() call in vendor_rust.py (line 921) fails with:

FileNotFoundError: [WinError 206] The filename or extension is too long

This is a Windows command-line length limit (~32K characters), not a file path length issue. The existing workarounds (core.longpaths, LongPathsEnabled registry) don't help here.

The issue is that vendor_rust.py passes all modified/added files to git add in a single command. When vendoring updates that touch many files, this exceeds Windows' command-line limit.

A fix would be to batch the git add calls in add_remove_files() to avoid hitting this limit.

Traceback:

File "vendor_rust.py", line 921, in vendor
    self.repository.add_remove_files(*extra_files, force=True)
File "mozversioncontrol/repo/git.py", line 244, in add_remove_files
    self._run(*cmd)
...
FileNotFoundError: [WinError 206] The filename or extension is too long

CI run: https://github.com/mozilla/neqo/actions/runs/21548242508/job/62092942263

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: