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

Use Bazelisk and update to Bazel 7.4.0 #61

Merged
merged 1 commit into from
Nov 1, 2024
Merged

Conversation

jjudd
Copy link

@jjudd jjudd commented Oct 29, 2024

No description provided.

@jjudd jjudd requested a review from jadenPete October 29, 2024 19:08
@@ -1,3 +1 @@
common --config=rules

Choose a reason for hiding this comment

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

Do you know why this existed to begin with?

Why have a rules section when we could just put those options under the commands themselves? E.g. instead of build:rules --disk_cache=.bazel_cache, we could do build --disk_cache=.bazel_cache.

Copy link
Author

Choose a reason for hiding this comment

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

These configs existed because way back when Bazel was new the tests and rules had separate config.

Also because of us wanting to have a single place for the disk cache, which I realize I broke by removing this. I'll deal with that.

.bazelrc_shared Outdated
@@ -22,14 +22,3 @@ test --test_output=all

build:rules --disk_cache=.bazel_cache

Choose a reason for hiding this comment

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

Will this option still take effect, now that we've removed the common --config=rules line?

@@ -1,3 +1 @@
common --config=rules
common:v7.2 --config=rules_v7.2

Choose a reason for hiding this comment

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

Do you know why this existed too?

Why not do foo:v7.2 bar instead of foo:rules_v7.2 bar?

@@ -22,14 +22,3 @@ test --test_output=all

build:rules --disk_cache=.bazel_cache
build:tests --disk_cache=../.bazel_cache

common:rules_v7.2 --config=noop

Choose a reason for hiding this comment

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

Do you know what this deleted portion was doing?

Copy link
Author

Choose a reason for hiding this comment

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

The per version config existed so we could set flags for certain versions of Bazel and not others. Back before 1.0 things would change often enough that we'd often need a different set of flags for each version.

Now that we're really only supporting 1 version of Bazel (because we only use these rules internally) we haven't had to do that for a while.

The noop config was a way to have a version config that did nothing. The version configs were automatically generated by the old tools/bazel, so you had to have them point to something, even if they did nothing.

tests/.bazelrc Outdated
@@ -1,5 +1,3 @@
common --config=tests

Choose a reason for hiding this comment

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

.bazelrc_shared still contains this line:

build:tests --disk_cache=../.bazel_cache

If delete common --config=tests, won't that option no longer take effect?

Copy link
Author

Choose a reason for hiding this comment

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

Yep. I missed that and will deal with this.

@jjudd jjudd merged commit 7f70ccc into lucid-master Nov 1, 2024
1 check 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