Add bazel 1 for backwards compatibility #81033
Conversation
mjlbach
changed the title
There was a problem hiding this comment.
I did a first review of this. There are a few things that we might want to adjust to make it more future proof (e.g. me adding bazel 0.25 right after this goes in).
Also we will have to squash and reword most of the commits to make them fit our contribution guide.
|
@andir Thanks for the review, still very new :) I'll squash my commits and clean up the message prior to its merge. I took care of the simpler things you suggested, and left some comments. |
ofborg
bot
added
10.rebuild-linux: 11-100
10.rebuild-linux: 1-10
and removed
10.rebuild-linux: 1-10
10.rebuild-linux: 11-100
labels
|
Just duplicate the whole expression instead of factoring out a common part. The expression for Bazel 2 should be able to move freely, and 1 should remain stable. |
|
@FRidh Would you want a separate subdirectory within pkgs/development/tools/build-managers/bazel for each bazel? Do we share patches? |
|
@FRidh here is my first crack at it https://github.com/mjlbach/nixpkgs/tree/bazel_separate_branches I was also thinking of creating a patch folder, or I can duplicate all of the patches for each version and put them in each folder... Happy to close and submit and alternate PR if that is what's best. |
|
I suggest calling the folder bazel 1 so there is no need to rename in case of a patch release. |
|
You can just push to this branch/PR. |
mjlbach
force-pushed
the
bazel_1
branch
2 times, most recently
from
41cfc75
to
e6a323b
Compare
|
@FRidh done and commits squashed. |
pkgs/development/tools/build-managers/bazel/bazel-1/default.nix
Outdated
Show resolved
Hide resolved
|
Testing this in combination with #80970. Will report back once TF finished compiling. |
|
TF did build (without CUDA; cuda build still running). It appears to be at least as function as before (still hitting #48902, so likely requires more work on my side to fix that). |
mjlbach
force-pushed
the
bazel_1
branch
2 times, most recently
from
d5742b6
to
13ad0fb
Compare
|
Is there anything else necessary for this PR? |
|
On 14:54 26.02.20, Michael Lingelbach wrote:
Is there anything else necessary for this PR?
A review from the maintainers would be nice (@Profpatsch). Otherwise I
think this is looking really good. Thank you for the work 👍.
|
|
Thank you for the guidance :) |
|
It looks fine to me. Usually I’m all for deduplication, but for these old version the main use case is being able to delete them very easily in the future, without creating dead code paths (or complications) in the current version. So yeah, whenever we update a major version let’s just copy the old one to a subdirectory, reference it from outside and be done with it. |
3 similar comments
|
It looks fine to me. Usually I’m all for deduplication, but for these old version the main use case is being able to delete them very easily in the future, without creating dead code paths (or complications) in the current version. So yeah, whenever we update a major version let’s just copy the old one to a subdirectory, reference it from outside and be done with it. |
|
It looks fine to me. Usually I’m all for deduplication, but for these old version the main use case is being able to delete them very easily in the future, without creating dead code paths (or complications) in the current version. So yeah, whenever we update a major version let’s just copy the old one to a subdirectory, reference it from outside and be done with it. |
|
It looks fine to me. Usually I’m all for deduplication, but for these old version the main use case is being able to delete them very easily in the future, without creating dead code paths (or complications) in the current version. So yeah, whenever we update a major version let’s just copy the old one to a subdirectory, reference it from outside and be done with it. |
|
Ready for final review |
|
@andir done! |
|
@GrahamcOfBorg build gvisor |
|
*renamed it to bazel-1 |
|
Why the rename? Versioned packages ought to be named with underscores preceding their major/minor numbers. |
|
I was making it consistent with the directory structure/didn't realize that was the trend. I reverted it. |
|
@GrahamcOfBorg build gvisor |
|
I think the bazel version before the update to 2.0 was succeeding on darwin, is this the same? |
|
I just backported changes from https://github.com/NixOS/nixpkgs/pull/80635/files which should fix the build on darwin. Testing now. |
|
@Profpatsch the latest builds on darwin |
Motivation for this change
Certain packages such as tensorflow build with older versions of bazel, this creates a selectable version bazel derivation. See #80970. I'm sure this could be cleaned up, so I welcome any feedback.
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)