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

feat: Allow environment variables in run configurations #5885

Merged
merged 8 commits into from
Jan 29, 2024

Conversation

blorente
Copy link
Collaborator

@blorente blorente commented Dec 20, 2023

Checklist

  • I have filed an issue about this change and discussed potential changes with the maintainers.
  • I have received the approval from the maintainers to make this change.
  • This is not a stylistic, refactoring, or cleanup change. (Well, a little bit)

Please note that the maintainers will not be reviewing this change until all checkboxes are ticked. See
the Contributions section in the README for more
details.

Discussion thread for this change

Issue Number: #2042
Replaces #4138

Description of this change

Allow users to set their own env vars per run-configuration. It is not respected everywhere (see the list below), but it works on our example projects.

Main features:

  • Env vars should work when runing and debugging any binary.
  • Env vars should work when running any test.
  • Env vars should work when debugging Go and Java tests (and probably Kotlin because it delegates to the Java runner, but I couldn't test it). Env vars for C++ test debugging are implemented internally, but they're part of a larger patch and I'll need some more work to untangle that (or maybe they're already covered in Pass environment variables from BlazeCidrLauncher down to bazel #5771)
  • Macro expansions (e.g. $WorkspacePath$) are not supported in Debug mode.

This work was based off of @victoriaroseb 's initial implementation.

Allow users to set their own env vars per run-configuration.
It is not respected everywhere (see the list below), but it works on our example projects.

Main features:
- Env vars should work when **run**ing and **debug**ging any binary.
- Env vars should work when **run**ning any test.
- Env vars should work when **debug**ging Go and Java tests. Env vars for C++ test debugging are implemented internally, but they're part of a larger patch and I'll need some more work to untangle that.
@github-actions github-actions bot added product: CLion CLion plugin product: IntelliJ IntelliJ plugin product: GoLand GoLand plugin awaiting-review Awaiting review from Bazel team on PRs labels Dec 20, 2023
@blorente
Copy link
Collaborator Author

This looks like a smallish change, but it's in a deep part of the codebase. I've tested it locally on the workflows listed, but please do give it a shot. It can use as many eyes as we can give it.

Copy link
Collaborator

@mai93 mai93 left a comment

Choose a reason for hiding this comment

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

I manually tested it for Java, it is not working when running a single test method but works when running the whole test class. I also think we need to also add unit tests.

Copy link
Collaborator Author

@blorente blorente left a comment

Choose a reason for hiding this comment

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

@mai93 Thanks for the comments! I've tried to address the inline ones there, but for the broader points:

I manually tested it for Java, it is not working when running a single test method but works when running the whole test class.

I've found that it works on a single method just fine (as long as I add the env vars to the new config as well). I've added example run configurations to the greetings_project to demonstrate, but please let me know if you still see that.

I also think we need to also add unit tests

I haven't done this yet, but will look into where it makes sense to add them. Thanks!

@blorente
Copy link
Collaborator Author

I have added a few unit tests, particularly for java tests. Unfortunately, I haven't seen any unit or integration tests for the biggest changes (namely, how the processes are actually spawned).
Please let me know if you'd like me to add more tests somewhere else.

Copy link
Collaborator

@mai93 mai93 left a comment

Choose a reason for hiding this comment

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

I left small comments and I tested it manually on Java, it is working fine. Thanks @blorente!

@@ -11,5 +11,5 @@ gazelle_target: //:gazelle
import_run_configurations:
# Test that validates that macro expansion works, as per issue:
# https://github.com/bazelbuild/intellij/issues/4112#event-7958662669
# This configuration should be executed on both 'run' (the green arrow) and 'debug' (the bug) modes.
# This configuration should only be executed on both 'run' (the green arrow) mode.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry if I misunderstood it, but in the PR description it says that Env vars should work for go debugging. has this changed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I should have written a better comment, sorry about that.
The main issue is that, because of how we pass env vars to debuggers, the IntelliJ macros (e.g. $ProjectName$) are not expanded. So, the env vars get passed, but the macros don't get expanded.

I've added a new run configuration to test env vars, which works on both run and debug.

@blorente
Copy link
Collaborator Author

@mai93 Thanks for the review! Comments addressed, please take a look when you have time.

@blorente blorente requested a review from mai93 January 25, 2024 10:44
@mai93
Copy link
Collaborator

mai93 commented Jan 27, 2024

Thanks @blorente! LGTM, leaving the merge for you if you want someone else to take a look as well.

@blorente
Copy link
Collaborator Author

Given the amount of time this has been active internally I'm pretty confident of merging after your review, but I'm happy to work on fixing issues (or writing documentation about where env vars do and don't work).

@blorente blorente merged commit c7a2a83 into bazelbuild:master Jan 29, 2024
6 checks passed
@github-actions github-actions bot removed the awaiting-review Awaiting review from Bazel team on PRs label Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product: CLion CLion plugin product: GoLand GoLand plugin product: IntelliJ IntelliJ plugin
Projects
Development

Successfully merging this pull request may close these issues.

3 participants