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.

Sign up for GitHub

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

Jump to bottom

Track getenv calls in module extensions #22498

Closed
wants to merge 2 commits into from
Closed

Track getenv calls in module extensions #22498

wants to merge 2 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented May 22, 2024

Fixes #22404

RELNOTES: Changes to environment variables read via getenv now correctly invalidate module extensions.

@fmeum fmeum removed the request for review from lberki May 22, 2024 15:00
@github-actions github-actions bot added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. awaiting-review PR is awaiting review from an assigned reviewer labels May 22, 2024
@fmeum fmeum force-pushed the 22404-getenv branch 2 times, most recently from 4dbe81f to 9c7f487 Compare May 22, 2024 17:17
fmeum
fmeum commented May 22, 2024
View reviewed changes
src/test/tools/bzlmod/MODULE.bazel.lock Outdated
@@ -82,13 +82,14 @@
}
}
},
"moduleExtensionMetadata": null,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure how we feel about null values popping up here.

Copy link
Member

Choose a reason for hiding this comment

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

Can you explain a bit why null ended up here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's because of
https://github.com/bazelbuild/bazel/pull/22498/files#diff-11c1c9cda0a3bafa6f377878111faecc4be3f83ac0a74fe9115f3cb297d78bf2R251

I could add a dedicated type adapter for Optional<String> to prevent this from happening for other Optional<T>. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think that would be a cleaner solution?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

@fmeum
Copy link
Collaborator Author

fmeum commented May 22, 2024

Tests should be passing now. This was unreasonably annoying due to having to deal with null values in Maps and GSON output, hope I didn't break anything else.

@fmeum fmeum force-pushed the 22404-getenv branch 2 times, most recently from ab1569c to 563742a Compare May 23, 2024 10:13
meteorcloudy
meteorcloudy approved these changes May 23, 2024
View reviewed changes
Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

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

Thanks!

@meteorcloudy meteorcloudy added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels May 23, 2024
@fmeum
Copy link
Collaborator Author

fmeum commented May 23, 2024

@bazel-io fork 7.2.0

@bazel-io bazel-io mentioned this pull request May 23, 2024
Closed
@fmeum
Copy link
Collaborator Author

fmeum commented May 24, 2024

@meteorcloudy The merge commit broke the lock file, should I force push a fix or are you taking care of that during import?

@meteorcloudy
Copy link
Member

We indeed had some trouble merging this one, updating this PR would definitely help! @iancha1992

fmeum added 2 commits May 24, 2024 11:00
@fmeum
Track getenv calls in module extensions
63a5af8 
RELNOTES: Changes to environment variables read via `getenv` now correctly invalidate module extensions.
@fmeum
Only serialize null for Optional<String>
4a999a7
@fmeum
Copy link
Collaborator Author

fmeum commented May 24, 2024

@meteorcloudy Should be resolved

@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label May 24, 2024
Wyverald pushed a commit that referenced this pull request May 25, 2024
@fmeum @Wyverald
Track getenv calls in module extensions
13908f9 
Fixes #22404

RELNOTES: Changes to environment variables read via `getenv` now correctly invalidate module extensions.

Closes #22498.

PiperOrigin-RevId: 637058337
Change-Id: Id4aaa4155a728452472eedae4a59c8d29456e512
@Wyverald Wyverald mentioned this pull request May 25, 2024
Merged
@fmeum fmeum deleted the 22404-getenv branch May 25, 2024 07:21
github-merge-queue bot pushed a commit that referenced this pull request May 28, 2024
@Wyverald @fmeum
[7.2.0] Track getenv calls in module extensions (#22541)
2f2d23d 
Fixes #22404

RELNOTES: Changes to environment variables read via `getenv` now
correctly invalidate module extensions.

Closes #22498.

PiperOrigin-RevId: 637058337
Change-Id: Id4aaa4155a728452472eedae4a59c8d29456e512

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meteorcloudy meteorcloudy meteorcloudy approved these changes

@Wyverald Wyverald Awaiting requested review from Wyverald Wyverald is a code owner

Assignees
No one assigned
Labels
team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

module extension not re-evaluated when env variable is modified
2 participants
@fmeum @meteorcloudy