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

Add a CI test for the new bzlmod integration #1617

Merged
merged 7 commits into from
Jul 3, 2023

Conversation

macandy13
Copy link
Contributor

Enhanced the already existing bazel test to also run with the --enable_bzlmod flag.

Technically it shouldn't be necessary to run these additional test on all operating systems, but this will make the eventual switch to exclusively use bzlmod (slightly) easier.

The one thing that is unclear is if there should be another test for the --define pfm=1 flag, which uses more libraries. This is missing for the base case and is currently only exercises in the cmake CI workflows, so it might be worth a separate PR to introduce those tests (for both variants then).

Related issue: bazelbuild/bazel-central-registry#384

@macandy13 macandy13 marked this pull request as ready for review June 30, 2023 07:20
@dmah42
Copy link
Member

dmah42 commented Jun 30, 2023

agreed, we should probably test pfm, but not in this PR.

i wonder if there's a way to simplify this by having a matrix-level setting for bzlmod and enabling or disabling the flag based on that?

@macandy13
Copy link
Contributor Author

macandy13 commented Jun 30, 2023 via email

@macandy13
Copy link
Contributor Author

Ok, I think I have a nice solution now (although the "&& ||" trick for emulating a ternary condition is definitely debatable ;). Also tried to incorporate pfm but got hit by the failing test with pfm-enabled, so I'll probably keep that for another PR where the test would be fixed as well.

PTAL.

@dmah42
Copy link
Member

dmah42 commented Jul 3, 2023

i need to update the "expected tests" to cover these too. merging without waiting for the non-existant tests.

@dmah42 dmah42 merged commit fed7337 into google:main Jul 3, 2023
57 of 60 checks passed
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.

2 participants