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

Bazel test shebangs #66723

Merged
merged 5 commits into from Oct 11, 2019
Merged

Bazel test shebangs #66723

merged 5 commits into from Oct 11, 2019

Conversation

aherrmann
Copy link
Member

As suggested by @Profpatsch in #66718 (comment) this PR adds a test to Bazel checking that shebangs have been patched appropriately.

This test flagged a few more instances of invalid python shebangs which are also fixed by this PR.

Motivation for this change

See #66718 (comment)

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 @Profpatsch

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.

Great, this removes another source of easy-to-make and hard-to-debug mistakes.

@Profpatsch
Copy link
Member

@GrahamcOfBorg build bazel.tests

Copy link
Member Author

@aherrmann aherrmann left a comment

Choose a reason for hiding this comment

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

@Profpatsch Thanks for the review, I've addressed your comments, PTAL.

@mboes mboes mentioned this pull request Sep 1, 2019
10 tasks
@kalbasit
Copy link
Member

kalbasit commented Sep 1, 2019

@GrahamcOfBorg build bazel bazel.tests

If test pass, I will merge so we can assert #67835 before merging.

@Profpatsch
Copy link
Member

@GrahamcOfBorg build bazel.tests

@Profpatsch
Copy link
Member

The Darwin build has lots of instances of this error:

ERROR: /private/tmp/nix-build-bazel-test-cpp.drv-1/wd/BUILD:1:1: Target '//:ProjectRunner' depends on toolchain '@local_config_cc//:cc-compiler-darwin', which cannot be found: error loading package '@local_config_cc//': Unable to find package for @rules_cc//cc:defs.bzl: The repository '@rules_cc' could not be resolved.'

@aherrmann
Copy link
Member Author

@Profpatsch also

ERROR: /private/tmp/nix-build-bazel-test-builtin-rules.drv-0/wd/python/BUILD.bazel:6:1: Target '//python:bin' depends on toolchain '@local_config_cc//:cc-compiler-darwin', which cannot be found: error loading package '@local_config_cc//': Unable to find package for @rules_cc//cc:defs.bzl: The repository '@rules_cc' could not be resolved.'
ERROR: Analysis of target '//python:bin' failed; build aborted: error loading package '@local_config_cc//': Unable to find package for @rules_cc//cc:defs.bzl: The repository '@rules_cc' could not be resolved.
INFO: Elapsed time: 9.871s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (12 packages loaded, 27 targets configured)
ERROR: Build failed. Not running target

That's strange, this PR doesn't touch @local_config_cc or @rules_cc. Do we know that this did not already happen before this PR?

@Profpatsch
Copy link
Member

Do we know that this did not already happen before this PR?

Good question, here’s a dummy PR to test: #67999

@Profpatsch
Copy link
Member

Update: The tests pass on current master.

@aherrmann
Copy link
Member Author

Update: The tests pass on current master.

That's grown to a pretty big diff by now. There's a merge conflict with master now anyway. I've rebased.

@aherrmann
Copy link
Member Author

@GrahamcOfBorg build bazel.tests

@Profpatsch
Copy link
Member

Profpatsch commented Sep 3, 2019

I think ofBorg isn’t listening to Members yet;
@GrahamcOfBorg build bazel.tests
Edit: one has to be a trusted user for ofborg to react: NixOS/ofborg@0377fd8

@Profpatsch
Copy link
Member

Hm, now there’s still one test failing on Darwin:

src/main/tools/process-wrapper-legacy.cc:58: "execvp(external/local_config_cc/cc_wrapper.sh, ...)": No such file or directory
INFO: Elapsed time: 3,476s, Critical Path: 0,06s
INFO: 0 processes.
FAILED: Build did NOT complete successfully
FAILED: Build did NOT complete successfully
builder for '/nix/store/lzii536cvmaix1bnavmwwn5liha3vjl8-bazel-test-cpp.drv' failed with exit code 1

@lheckemann lheckemann added this to the 20.03 milestone Sep 10, 2019
@Profpatsch
Copy link
Member

Ping. So close :)

@flokli flokli mentioned this pull request Oct 11, 2019
10 tasks
@flokli
Copy link
Contributor

flokli commented Oct 11, 2019

rebased on latest master.

@flokli
Copy link
Contributor

flokli commented Oct 11, 2019

@GrahamcOfBorg build bazel.tests

@flokli flokli merged commit 74008e2 into NixOS:master Oct 11, 2019
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