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

rustc: add QA documentation #73296

Merged
merged 1 commit into from Dec 20, 2019
Merged

rustc: add QA documentation #73296

merged 1 commit into from Dec 20, 2019

Conversation

Mic92
Copy link
Member

@Mic92 Mic92 commented Nov 12, 2019

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@Mic92 Mic92 requested a review from LnL7 as a code owner November 12, 2019 15:54
@Mic92
Copy link
Member Author

Mic92 commented Nov 12, 2019

cc @andir @edolstra

@@ -1,3 +1,8 @@
# After updating check ...
# 1. if rustc can produce rust binaries on x86_64-linux, aarch64-linux and x86_64-darwin:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't really agree with this. For example, I have no ability to test on aarch64 or macOS.

Copy link
Member Author

@Mic92 Mic92 Nov 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GrahamcOfBorg usually can build this in time. For aarch64 we have a beefy community builder: https://github.com/nix-community/aarch64-build-box
which I have access to and usually try to build upgrades there and post the result back to the pr thread. For macOS alternatively I have a bunch of nixpkgs maintainer that I can ping instead. We followed this pattern for a while until you started merging rust upgrades without testing.

@@ -1,3 +1,8 @@
# After updating check ...
# 1. if rustc can produce rust binaries on x86_64-linux, aarch64-linux and x86_64-darwin:
# i.e. nix build -f . fd or @GrahamcOfBorg build fd on github
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does fd mean? And to what directory does -f . refer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fd is just a simple rust example that should build on all platforms.

# 1. if rustc can produce rust binaries on x86_64-linux, aarch64-linux and x86_64-darwin:
# i.e. nix build -f . fd or @GrahamcOfBorg build fd on github
# 2. if our LLVM version matches with rust upstream
# 3. if firefox or thunderbird still build on x86_64-linux
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the implications of this? E.g. if firefox fails to build, should we simply refrain from updating Rust?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Because this is a channel blocker. A way to fix it in that case is to to pin rust to an older version for firefox. However breaking firefox in unstable is undesired.

@Mic92
Copy link
Member Author

Mic92 commented Nov 12, 2019

Other usual suspects in rust land: cc @symphorien @jD91mZM2 @zimbatm

@worldofpeace
Copy link
Contributor

Is there any particular reason why rustc updates have been merged to master?

@Mic92
Copy link
Member Author

Mic92 commented Nov 12, 2019

Yeah. Maybe I should add this as well, we usually prefer to merge rustc updates to staging.

@worldofpeace
Copy link
Contributor

Yeah. Maybe I should add this as well, we usually prefer to merge rustc updates to staging.

Thanks, we should be doing this always. I can recall at least two occasions where this has taken up my time because there were urgent failures on master because of a rustc update. When it's on staging it isn't as urgent and we can fix it during its workflow.

@Mic92
Copy link
Member Author

Mic92 commented Nov 13, 2019

It now mentions, that rust updates should go to staging first and that one can ask other people for help to test rust on other platforms. In my experience this testing takes less than a week depending on how fast people respond but it is worth it to avoid unnecessary breakages in master.

@worldofpeace
Copy link
Contributor

It now mentions, that rust updates should go to staging first and that one can ask other people for help to test rust on other platforms. In my experience this testing takes less than a week depending on how fast people respond but it is worth it to avoid unnecessary breakages in master.

Oh, I know this my be rather nitpicky, but maybe just having a README.md or QA-Instructions.md in the dir would be better?

@jD91mZM2
Copy link
Member

I think it makes sense to have it inline, as you can't possibly avoid opening that file when updating rust. I'm normally somewhat of an expert at failing to read documentation ;)

@LnL7
Copy link
Member

LnL7 commented Nov 13, 2019

Until recently we've only kept a single version of rust around. With 2 or more versions we could keep the default on a version compatible with firefox as well as make introducing versions much less intrusive. Similar to what we do for most other compilers/interpreters.

@Mic92 Mic92 merged commit d68c70f into NixOS:master Dec 20, 2019
@Mic92 Mic92 deleted the rustc branch December 20, 2019 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants