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

Make jar_app_layer._classpath_as_file public #1947

Merged
merged 12 commits into from
Nov 10, 2021

Conversation

jakemcc
Copy link
Contributor

@jakemcc jakemcc commented Nov 1, 2021

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Callers of jar_app_layer can not specify _classpath_as_file.

What is the new behavior?

Yes. It changes the jar_app_layer._classpath_as_file private attribute to a public attribute jar_app_layer.classpath_as_file. This allows callers to override the default value of False when applying the jar_app_layer rule.

The PR also make the callers of jar_app_layer expose an optional classpath_as_file argument so that folks can passthrough the argument to jar_app_layer.

Some details of why someone would want to do this can be found in the Bazel slack

Does this PR introduce a breaking change?

  • Yes
  • No

Given that the classpath_as_file was private before, folks couldn't pass it in before so I would think that changing from private to public is a non-breaking change. The other changes to the various jvm *_image macros keep the previous behavior as the default.

Other information

@google-cla google-cla bot added the cla: yes label Nov 1, 2021
@jakemcc jakemcc marked this pull request as ready for review November 2, 2021 15:45
Copy link
Collaborator

@gravypod gravypod left a comment

Choose a reason for hiding this comment

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

I think this looks good. The problems with the Mac runner as known of but I'm not sure what causes them. If your team uses macs this may be something worth looking into but I don't think this will cause any regressions.

@gravypod
Copy link
Collaborator

gravypod commented Nov 2, 2021

It looks like some of the non-hermetic tests are breaking because an upstream dep changed:





==================== Test output for //tests/docker/package_managers:install_pkgs_reproducibility_test:
  | Traceback (most recent call last):
  | File "/var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/sandbox/linux-sandbox/1862/execroot/io_bazel_rules_docker/bazel-out/k8-fastbuild/bin/tests/docker/package_managers/install_pkgs_reproducibility_test.runfiles/io_bazel_rules_docker/contrib/compare_ids_test.py", line 63, in <module>
  | compare_ids(args.tars, args.id)
  | File "/var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/sandbox/linux-sandbox/1862/execroot/io_bazel_rules_docker/bazel-out/k8-fastbuild/bin/tests/docker/package_managers/install_pkgs_reproducibility_test.runfiles/io_bazel_rules_docker/contrib/compare_ids_test.py", line 51, in compare_ids
  | id_
  | RuntimeError: Digest mismatch: actual sha256:99481007ec6e78c605b9a2ce8ff4dacd8c5caa804a33ebf693b4609fe44e7a15 vs expected sha256:891f28dbc25f7785db1ce36e54a37e1bc1af3b4dcfd9d07b409905c35b85444c
  | ================================================================================
  | ==================== Test output for //tests/docker/package_managers:install_pkgs_reproducibility_test:
  | Traceback (most recent call last):
  | File "/var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/sandbox/linux-sandbox/1864/execroot/io_bazel_rules_docker/bazel-out/k8-fastbuild/bin/tests/docker/package_managers/install_pkgs_reproducibility_test.runfiles/io_bazel_rules_docker/contrib/compare_ids_test.py", line 63, in <module>
  | compare_ids(args.tars, args.id)
  | File "/var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/sandbox/linux-sandbox/1864/execroot/io_bazel_rules_docker/bazel-out/k8-fastbuild/bin/tests/docker/package_managers/install_pkgs_reproducibility_test.runfiles/io_bazel_rules_docker/contrib/compare_ids_test.py", line 51, in compare_ids
  | id_
  | RuntimeError: Digest mismatch: actual sha256:99481007ec6e78c605b9a2ce8ff4dacd8c5caa804a33ebf693b4609fe44e7a15 vs expected sha256:891f28dbc25f7785db1ce36e54a37e1bc1af3b4dcfd9d07b409905c35b85444c
  | ================================================================================
  | ==================== Test output for //tests/docker/package_managers:install_pkgs_reproducibility_test:
  | Traceback (most recent call last):
  | File "/var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/sandbox/linux-sandbox/1866/execroot/io_bazel_rules_docker/bazel-out/k8-fastbuild/bin/tests/docker/package_managers/install_pkgs_reproducibility_test.runfiles/io_bazel_rules_docker/contrib/compare_ids_test.py", line 63, in <module>
  | compare_ids(args.tars, args.id)
  | File "/var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/sandbox/linux-sandbox/1866/execroot/io_bazel_rules_docker/bazel-out/k8-fastbuild/bin/tests/docker/package_managers/install_pkgs_reproducibility_test.runfiles/io_bazel_rules_docker/contrib/compare_ids_test.py", line 51, in compare_ids
  | id_
  | RuntimeError: Digest mismatch: actual sha256:99481007ec6e78c605b9a2ce8ff4dacd8c5caa804a33ebf693b4609fe44e7a15 vs expected sha256:891f28dbc25f7785db1ce36e54a37e1bc1af3b4dcfd9d07b409905c35b85444c
 

<br class="Apple-interchange-newline">```

@jakemcc
Copy link
Contributor Author

jakemcc commented Nov 3, 2021

Retriggered the build and looks like we're back to just the macos builds failing.

@gravypod
Copy link
Collaborator

gravypod commented Nov 7, 2021

@jakemcc , sorry for the delay. Unfortunately, I don't have access to mac hardware so I've disabled the newly failing tests on mac. I'll fix any issues that come up from this change.

@gravypod
Copy link
Collaborator

gravypod commented Nov 9, 2021

Sorry for the delay. I recently pushed a PR to disable the newly broken tests in CI here: a351a3f

If you rebase past this one things should be good. Very sorry about the inconvenience this has caused you!

@jakemcc jakemcc force-pushed the jm.expose_classpath_as_file branch from e7a008c to ff17515 Compare November 9, 2021 17:39
@jakemcc
Copy link
Contributor Author

jakemcc commented Nov 9, 2021

Sorry for the delay. I recently pushed a PR to disable the newly broken tests in CI here: a351a3f

If you rebase past this one things should be good. Very sorry about the inconvenience this has caused you!

No worries. Thanks for sticking with it and helping out!

@alexeagle alexeagle merged commit 8f6a2aa into bazelbuild:master Nov 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants