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

Update Bazel 2.1.0 --> 3.3.1 #6761

Merged
merged 15 commits into from
Jul 23, 2020
Merged

Update Bazel 2.1.0 --> 3.3.1 #6761

merged 15 commits into from
Jul 23, 2020

Conversation

aherrmann-da
Copy link
Contributor

@aherrmann-da aherrmann-da commented Jul 16, 2020

  • Updates the nixpkgs revision to update Bazel.
    • Removes the now unused minio which failed to build on MacOS after the update.
    • Fixes hlint warnings due to the more recent version of hlint provided by the more recent nixpkgs revision.
  • Updates Bazel on Windows
  • Updates rules_nixpkgs - the hermetic cc toolchain had to be updated for Bazel 3.3.1
  • --noincompatible_windows_native_test_wrapper was removed in Bazel.
    We had some custom test rules that wrote an executable shell script .sh. This worked because Bazel executed tests in a shell wrapper. In recent versions this is no longer the case on Windows and tests have to be standalone executables.
    To work around this issue this introduces a more general sh_inline_test rule that allows to specify a test shell script as an attribute. Internally it combines a dedicated rule that writes the script into a file and passes it to the builtin sh_test rule.
    This triggered sh_binary has multiple outputs in some contexts bazelbuild/bazel#11820 and required an additional workaround to find the .exe output sh_binary client's of client_server_test on Windows.
  • This bumps the Bazel cache URL on Windows and runs clean --expunge at the beginning to avoid interference with the previous Bazel version. The clean --expunge can be removed once this is merged.

Pull Request Checklist

  • Read and understand the contribution guidelines
  • Include appropriate tests
  • Set a descriptive title and thorough description
  • Add a reference to the issue this PR will solve, if appropriate
  • Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
  • Normal production system change, include purpose of change in description

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

@aherrmann-da aherrmann-da force-pushed the bazel-update branch 3 times, most recently from 1e72a5c to d015dd5 Compare July 17, 2020 13:24
@aherrmann-da aherrmann-da force-pushed the bazel-update branch 8 times, most recently from 8e8d3ad to c744ef9 Compare July 22, 2020 09:45
aherrmann added 13 commits July 22, 2020 15:06
It used to be used as a gateway to push the Nix cache to GCS, but has
since been replaced by nix-store-gcs-proxy.
changelog_begin
changelog_end
The nixpkgs update implied an hlint update which enabled new warnings.
Since Bazel 2.2.0 the order of generating `WORKSPACE` and `BUILD` files
and applying patches has been reversed. The allows users to define
patches to these files that will not be immediately overwritten.
However, it also means that patches on another repository's original
`WORKSPACE` file will likely become invalid.

* bazelbuild/bazel@a948eb7
* bazelbuild/bazel#10681

Hint: If you're generating a patch with `git` then you can use the
following command to exclude the `WORKSPACE` file.

```
git diff ':(exclude)WORKSPACE'
```
client_server_test used to produce an executable shell script in form of
a text file output. However, since the removal of
`--noincompatible_windows_native_test_wrapper` this no longer works on
Windows since `.sh` files are not directly executable on Windows.

This change fixes the issue by producing the script file in a dedicated
rule and then wrapping it in a `sh_test` rule which also works on
Windows.
@aherrmann-da aherrmann-da marked this pull request as ready for review July 22, 2020 16:10
Copy link
Contributor

@cocreature cocreature left a comment

Choose a reason for hiding this comment

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

Great work, thank you so much!

@@ -200,71 +200,23 @@ index 721f64d..3ee682b 100644
Example:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be great to get this merged upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, upstream PRs are here tweag/rules_nixpkgs#132, tweag/rules_nixpkgs#128. I've factored out the nixopts location expansion into its own PR to ease review.

@@ -41,14 +41,14 @@ data Tools = Tools

newtype DamlOption = DamlOption FilePath
instance IsOption DamlOption where
defaultValue = DamlOption $ "daml"
defaultValue = DamlOption "daml"
Copy link
Contributor

Choose a reason for hiding this comment

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

huh, I wonder what caused hlint to not catch this before, maybe haskell-src-exts just couldn’t parse anything in this file.

@aherrmann-da aherrmann-da merged commit 4b14382 into master Jul 23, 2020
@aherrmann-da aherrmann-da deleted the bazel-update branch July 23, 2020 07:46
garyverhaegen-da added a commit that referenced this pull request Jul 23, 2020
Following #6761, all nodes have been reset, so we can get back to fast,
cached builds.

CHANGELOG_BEGIN
CHANGELOG_END
garyverhaegen-da added a commit that referenced this pull request Jul 24, 2020
Following #6761, all nodes have been reset, so we can get back to fast,
cached builds.

CHANGELOG_BEGIN
CHANGELOG_END
garyverhaegen-da added a commit that referenced this pull request Sep 4, 2020
It looks like the structure of the nix package has changed when we
updated nixpkgs in #6761, so we need to update the dev-env script to
match.

CHANGELOG_BEGIN
CHANGELOG_END
@garyverhaegen-da garyverhaegen-da mentioned this pull request Sep 4, 2020
garyverhaegen-da added a commit that referenced this pull request Sep 4, 2020
It looks like the structure of the nix package has changed when we
updated nixpkgs in #6761, so we need to update the dev-env script to
match.

CHANGELOG_BEGIN
CHANGELOG_END
garyverhaegen-da added a commit that referenced this pull request Sep 10, 2020
It looks like #6761 broke our Terraform setup by upgrading the nixpkgs
snapshot. That this has not been caught earlier is, I suppose, a
testament to how stable our infrastructure has become nowadays.

This is the same issue we had with the Google providers in #6402, i.e.
we are trying to pin the provider versions both at the nix level and at
the terraform level, with no way to force them to stay in sync.

I don't have a good proposal for such a way, and it seems rare and
innocuous enough to not warrant the investment to fix this at a more
fundamental level.

CHANGELOG_BEGIN
CHANGELOG_END
garyverhaegen-da added a commit that referenced this pull request Sep 10, 2020
It looks like #6761 broke our Terraform setup by upgrading the nixpkgs
snapshot. That this has not been caught earlier is, I suppose, a
testament to how stable our infrastructure has become nowadays.

This is the same issue we had with the Google providers in #6402, i.e.
we are trying to pin the provider versions both at the nix level and at
the terraform level, with no way to force them to stay in sync.

I don't have a good proposal for such a way, and it seems rare and
innocuous enough to not warrant the investment to fix this at a more
fundamental level.

CHANGELOG_BEGIN
CHANGELOG_END
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants