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.

Sign up for GitHub

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

Jump to bottom

bazel: 2.0.0 -> 2.1.0 #80739

Merged
merged 2 commits into from Feb 28, 2020
Merged

bazel: 2.0.0 -> 2.1.0 #80739

merged 2 commits into from Feb 28, 2020

Conversation

jpgneves
Copy link
Contributor

Motivation for this change

The official Bazel wrapper script included from version 2.0.0 onwards enforces the actual bazel binary (previously named bazel-real in this package) has the following naming convention, if the root of your Bazel project contains a .bazelversion file: bazel-${version}-${os}-${arch}.

This modifies output binary name and, while we're at it, upgrades Bazel to 2.1.0.

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 nixpkgs-review --run "nixpkgs-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.

@ofborg ofborg bot requested a review from mboes February 21, 2020 16:40
@uri-canva
Copy link
Contributor

Looking at the current implementation of the script on master, it doesn't enforce that naming convention for the binary, it still supports calling it bazel-real as well: https://github.com/bazelbuild/bazel/blob/59d7864b3a39008e6b4d1447abcdc59cd9906e88/scripts/packages/bazel.sh

In that case the .bazelversion becomes a check that makes sure the version you're running matches it.

@Profpatsch
Copy link
Member

Profpatsch commented Feb 24, 2020 •
edited

In that case the .bazelversion becomes a check that makes sure the version you're running matches it.

That looks like what we should be doing.

@andir
Copy link
Member

andir commented Feb 24, 2020

Please don't just update this version. Does 2.0 remain compatible with 2.1 in all cases? If not we should consider having another attribute within nixpkgs. The update to Bazel 2.0 already caused some breakage that we didn't get rid of yet. See the relevant conversations in #76851 and my WIP PR #80953.

@Profpatsch
Copy link
Member

bazel promises no backwards-compatible changes for three months at a time

If they release a breaking change, they bump the major version.

@Profpatsch
Copy link
Member

Profpatsch commented Feb 24, 2020 •
edited

This modifies output binary name and, while we're at it, upgrades Bazel to 2.1.0.

Let’s put this in another commit.

@jpgneves
Copy link
Contributor Author

Will do! Apologies for the messy intermixing of changes. Should I close this PR and resubmit separate ones, or is it sufficient to create a new one with the version change?

@Profpatsch
Copy link
Member

Should I close this PR and resubmit separate ones, or is it sufficient to create a new one with the version change?

It’s fine if you pull it out in a separate commit, they can both be in this PR.

@Profpatsch
Copy link
Member

related comment in the original issue:

#80950 (comment)

@Profpatsch
Copy link
Member

So I guess as long as we have a normal bazel binary and the versioning is what bazel expects, it’s better to be as specific as possible.

Profpatsch
Profpatsch approved these changes Feb 25, 2020
View reviewed changes
Copy link
Member

@Profpatsch Profpatsch left a comment

Choose a reason for hiding this comment

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

Once the two commits are split, this is good to go. 👍

@Profpatsch Profpatsch mentioned this pull request Feb 25, 2020
Merged
@jpgneves jpgneves force-pushed the bazel-2.1.0 branch 2 times, most recently from d7166ba to 124196f Compare February 26, 2020 08:49
@Profpatsch
Copy link
Member

@GrahamcOfBorg build bazel.tests

@jpgneves
Copy link
Contributor Author

Regarding the failed tests on darwin, I do not have a machine where I can work to fix the issue, so I'd appreciate any assistance in figuring it out, if it's a blocker (which it sounds like it is). :)

@Profpatsch
Copy link
Member

@kalbasit you seem to have fixed the protobuf test last time, could you take a look?

@uri-canva
Copy link
Contributor

The failure I'm getting on darwin is the one that was fixed in #80635.

@uri-canva
Copy link
Contributor

Rebased on current master (b1ec189) it works.

@jpgneves
bazel: 2.0.0 -> 2.1.0
351dc96
@jpgneves
bazel: output correct binary name
3ef9a18 
From Bazel 2.0.0 onwards, Bazel looks for a binary named
`bazel-${version}-${os_arch}` if the project root contains a
`.bazelversion` file or the USE_BAZEL_VERSION environment
variable is set.

This change ensures we output a binary with the correct name
for the current version and OS/arch combination.
@jpgneves
Copy link
Contributor Author

Rebased it on current master right now.

@Profpatsch
Copy link
Member

cool

@Profpatsch Profpatsch merged commit 95c91ce into NixOS:master Feb 28, 2020
@andir andir mentioned this pull request Mar 1, 2020
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Profpatsch Profpatsch Profpatsch approved these changes

@mboes mboes Awaiting requested review from mboes

Assignees
No one assigned
Labels
10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jpgneves @uri-canva @Profpatsch @andir @Janik-Haag