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

Resolve env variables in bazelrc #13266

Closed
wants to merge 1 commit into from
Closed

Conversation

anthpi
Copy link
Contributor

@anthpi anthpi commented Mar 25, 2021

Bazel will now try to resolve environment variables
provided in bazelrc files.
If environmetn variable is not found text will remain
unchanged.

Format to be used: ${FOO}

Example:
import ${GIT_ROOT}/some/path/bazel.rc

@google-cla google-cla bot added the cla: yes label Mar 25, 2021
@anthpi
Copy link
Contributor Author

anthpi commented Mar 25, 2021

This commit is related to the discussion in #10904

@anthpi anthpi marked this pull request as draft March 26, 2021 07:42
@anthpi anthpi force-pushed the master branch 4 times, most recently from fffa2fb to b3a5375 Compare March 26, 2021 08:37
@anthpi anthpi marked this pull request as ready for review March 29, 2021 07:20
@anthpi anthpi marked this pull request as draft March 29, 2021 14:38
@anthpi anthpi force-pushed the master branch 2 times, most recently from 2b945b7 to 39f54e9 Compare March 29, 2021 16:21
@aiuto
Copy link
Contributor

aiuto commented Mar 31, 2021

We never resolve in #10904 if this is a good idea or not.
@lberki I'm passing this off to you to make a judgment call on the feature in general.

Bazel will now try to resolve environment variables
provided in bazelrc files.
If environmetn variable is not found text will remain
unchanged.

Format to be used: ${FOO}

Example:
  import ${GIT_ROOT}/some/path/bazel.rc
@anthpi
Copy link
Contributor Author

anthpi commented Mar 31, 2021

@aiuto I think there are no opinions against the feature (a part from your comment about added complexity). Feature is optional to use and backwards compatible so it should not cause any issues for the existing users.

@lberki For some reason tests on CentOS are still failing and I was not able to identify why. I had similar issues with Windows platform tests and I was able to resolve them by using "blaze::GetEnv()" method that should take care of platform specific implementations. So my guess is that error probably related to the test case rather than to implementation. At the same time CentOS is a Linux platform and I was not able to find any special "ifdefs" that are CentOS specific. I locally run RHEL7 and tests pass. They even pass in Ubuntu environment while being tested by Buildkite. That's why I'm really puzzled about tests failing on CentOS.

@lberki Overall implementation of the feature is not very complex and located in one place. I hope that it should not be hard to maintain it. Feature is backwards compatible and optional to use. It will solve many issues that we have in our environment when we have customers running several workspaces in several repos. In #10904 there were several people mentioning having a bazel wrapper just to be able to achieve same result. Today you either need to have a wrapper or provide parameters on the command line to be able to resolve env variables. Both of those things make environment complex and cumbersome to use for the users. With this feature in place one can remove the unnecessary wrappers and move over many (if not most) of the flags into the .bazelrc file thus greatly simplifying bazel commands that users need to type.

@lberki It's "by design" that format is not platform specific to allow portability between the platforms (if there is such a use case). Also, after discussing internally with my colleagues, we came to a conclusion that keeping values unresolved when required environment variables are not found is probably the best solution because this way users will get a clear error message containing actual name of the environment variable to be able to troubleshoot and fix it. Both of the things are debatable of course and I would like to get your feedback on this. By making these assumptions we were also able to simplify implementation.

@anthpi anthpi marked this pull request as ready for review March 31, 2021 05:56
@anthpi
Copy link
Contributor Author

anthpi commented Apr 13, 2021

@lberki Did you have time to start looking into this pull request?

@lberki
Copy link
Contributor

lberki commented Apr 13, 2021

Erm, sorry for the delay. I answered on #10904 .

@sgowroji sgowroji added awaiting-user-response Awaiting a response from the author team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website and removed type: feature request labels Oct 19, 2022
@keertk
Copy link
Member

keertk commented Dec 7, 2022

Hi there! Thank you for contributing to the Bazel repository. We appreciate your time and effort. We're doing a clean up of old PRs and will be closing this one since it seems to have stalled. Please feel free to reopen if you’re still interested in pursuing this or if you'd like to discuss anything further. We’ll respond as soon as we have the bandwidth/resources to do so.

@keertk keertk closed this Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-user-response Awaiting a response from the author cla: yes team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants