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

chore: remove experimental_strict_action_env option from bazelrc #13200

Merged

Conversation

LKreutzer
Copy link
Contributor

@LKreutzer LKreutzer commented Jul 5, 2022

Signed-off-by: Lars Kreutzer lars.kreutzer@tngtech.com

Summary

  • The bazel option --experimental_strict_action_env is removed from the .bazelrc
  • This option was introduced in feat: Add wrapper Make commands to run Bazel #10642 to wrap some bazel commands in Make targets
    • The Make targets do not seem to be used in CI but may be convenient for local development

Test Plan

  • CI
  • Run the Make targets build_c, test_c and build_oai_clang.
    • The Make calls need to happen in magma/lte/gateway, however there is currently a bug (see issue Bazel commands only work in the magma directory #13102) which requires a slight modification of the .bazelrc for testing.
    • The line import bazel/bazelrcs/cache.bazelrc needs to be replaced by import /home/vagrant/magma/bazel/bazelrcs/cache.bazelrc for testing inside the magma-vm.
    • Builds are successful and no change in behaviour is observed compared to the run with the flag.

Additional Information

  • The initial run of the bazel workflow is very slow on this PR because the removal of the flag prevents most cache hits. Once the cache is filled on master the build time should be unaffected.

    • I filled the cache from the fork and now the workflow has a normal runtime.
  • The option has been renamed to incompatible_strict_action_env (see --incompatible_strict_action_env bazelbuild/bazel#6648)

  • The option has the following description in the current docs:
    Screenshot from 2022-07-05 16-35-06

    • Since we have a relatively reproducible build environment in CI it should not affect the cache hits once they are generated without this flag.
    • ⚠️ It might be good to monitor the remote cache hits in the weeks following a potential merge of this PR.
  • We specify build:asan --action_env=ASAN_OPTIONS=detect_leaks=1:color=always in the current .bazelrc

  • This change is backwards-breaking

Signed-off-by: Lars Kreutzer <lars.kreutzer@tngtech.com>
@LKreutzer LKreutzer added the bazel changes for the Bazelification effort label Jul 5, 2022
@LKreutzer LKreutzer self-assigned this Jul 5, 2022
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines. label Jul 5, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2022

Thanks for opening a PR! 💯

A couple initial guidelines

Howto

  • Reviews. The "Reviewers" listed for this PR are the Magma maintainers who will shepherd it.
  • Checks. All required CI checks must pass before merge.
  • Merge. Once approved and passing CI checks, use the ready2merge label to indicate the maintainers can merge your PR.

More info

Please take a moment to read through the Magma project's

If this is your first Magma PR, also consider reading

@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2022

agw-workflow

580 tests   576 ✔️  3m 48s ⏱️
    2 suites      4 💤
    2 files        0

Results for commit 7ac3332.

@LKreutzer LKreutzer changed the title chore: TESTING/POC remove experimental_strict_action_env option from bazelrc chore: remove experimental_strict_action_env option from bazelrc Jul 6, 2022
@LKreutzer LKreutzer marked this pull request as ready for review July 6, 2022 10:41
@LKreutzer LKreutzer requested review from a team and nstng July 6, 2022 10:41
@LKreutzer LKreutzer added the ready2merge This PR is ready to be merged (is approved and passes all required checks) label Jul 6, 2022
Copy link
Contributor

@nstng nstng left a comment

Choose a reason for hiding this comment

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

LGTM - discussed offline

@LKreutzer LKreutzer merged commit 5de4aaf into magma:master Jul 6, 2022
emakeev pushed a commit to emakeev/magma that referenced this pull request Aug 5, 2022
…ma#13200)

Signed-off-by: Lars Kreutzer <lars.kreutzer@tngtech.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bazel changes for the Bazelification effort ready2merge This PR is ready to be merged (is approved and passes all required checks) size/XS Denotes a PR that changes 0-9 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants