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

fix(native_image): fix C++ toolchain env setup #72

Closed
wants to merge 3 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Aug 31, 2023

Instead of relying on the default shell env, get the environment
variables declared by the C++ toolchain. On macOS, additionally use
apple_support to pass in DEVELOPER_DIR.

Since GraalVM sanitizes the environment, all variables are translated
into -E flags.

Fixes #67

fmeum added 3 commits August 31, 2023 08:54
Make use of `Args` lazy string formatting to save memory.
Also pass in compiler options as repeated flags, not joined on spaces,
to prevent issues when compiler options contain spaces.
`depset` structures should be kept as flat as possible. C++ toolchain
files are also already `depset`s and thus shouldn't be passed into
`tools`.
Instead of relying on the default shell env, get the environment
variables declared by the C++ toolchain. On macOS, additionally use
`apple_support` to pass in `DEVELOPER_DIR`.

Since GraalVM sanitizes the environment, all variables are translated
into `-E` flags.
@fmeum fmeum requested a review from sgammon as a code owner August 31, 2023 13:18
@fmeum
Copy link
Collaborator Author

fmeum commented Aug 31, 2023

Stacked on #70 and #71

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@fmeum
Copy link
Collaborator Author

fmeum commented Aug 31, 2023

@keith This is what I came up with based on your diff. It works in Bazel CI :⁠-⁠)

@sgammon sgammon added bug Something isn't working enhancement New feature or request labels Sep 1, 2023
@sgammon sgammon added this to the 1.0.0 milestone Sep 1, 2023
@sgammon sgammon added 🚧 WIP Work-in-progress, do not merge platform:darwin Issues relating to macOS platform:windows Issues relating to Windows and MSVC labels Sep 1, 2023
sgammon added a commit that referenced this pull request Sep 1, 2023
- fix: use legacy rules from legacy gvm
- chore: drop `MODULE-resolved.bzl`
- chore: update bzlmod lock
- chore: update lib/docs deps and rebuild docs
- chore: remove redundant calls in sample projects

Applied on top of #72

Signed-off-by: Sam Gammon <sam@elide.ventures>
sgammon added a commit that referenced this pull request Sep 1, 2023
- fix: use legacy rules from legacy gvm
- chore: drop `MODULE-resolved.bzl`
- chore: update bzlmod lock
- chore: update lib/docs deps and rebuild docs
- chore: remove redundant calls in sample projects

Applied on top of #72

Signed-off-by: Sam Gammon <sam@elide.ventures>
@sgammon sgammon mentioned this pull request Sep 1, 2023
@fmeum
Copy link
Collaborator Author

fmeum commented Sep 1, 2023

I can see how lambda and nested functions in general are problematic on Bazel 4. Unfortunately loading them conditionally isn't possible without either splitting out the non-legacy rules or using a Bazel version check in a repository rule and generating code.

We could also mimic what apple_support.run does and then directly control the way environment variables are passed to ctx.actions.run. Maybe @keith also knows a better solution.

@sgammon
Copy link
Owner

sgammon commented Sep 1, 2023

@fmeum see #80, i've performed the rule split and refactor in anticipation of that same problem. bazel4 is fixed, mac tests are working, and that PR now includes this one as well as #73

i hope we can move discussion to #80 where we can prep a release at 0.10.x 😄

as such closing this (but only as long as that PR works for your needs @fmeum -- if you hit issues we can always reopen this one.)

@sgammon sgammon closed this Sep 1, 2023
@sgammon sgammon removed the 🚧 WIP Work-in-progress, do not merge label Sep 1, 2023
@fmeum fmeum deleted the env branch September 1, 2023 09:09
sgammon added a commit that referenced this pull request Sep 2, 2023
- fix: use legacy rules from legacy gvm
- chore: drop `MODULE-resolved.bzl`
- chore: update bzlmod lock
- chore: update lib/docs deps and rebuild docs
- chore: remove redundant calls in sample projects

Applied on top of #72

Signed-off-by: Sam Gammon <sam@elide.ventures>
sgammon added a commit that referenced this pull request Sep 4, 2023
- fix: use legacy rules from legacy gvm
- chore: drop `MODULE-resolved.bzl`
- chore: update bzlmod lock
- chore: update lib/docs deps and rebuild docs
- chore: remove redundant calls in sample projects

Applied on top of #72

Signed-off-by: Sam Gammon <sam@elide.ventures>
sgammon added a commit that referenced this pull request Sep 8, 2023
- fix: use legacy rules from legacy gvm
- chore: drop `MODULE-resolved.bzl`
- chore: update bzlmod lock
- chore: update lib/docs deps and rebuild docs
- chore: remove redundant calls in sample projects

Applied on top of #72

Signed-off-by: Sam Gammon <sam@elide.ventures>
sgammon added a commit that referenced this pull request Sep 8, 2023
- fix: use legacy rules from legacy gvm
- chore: drop `MODULE-resolved.bzl`
- chore: update bzlmod lock
- chore: update lib/docs deps and rebuild docs
- chore: remove redundant calls in sample projects

Applied on top of #72

Signed-off-by: Sam Gammon <sam@elide.ventures>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request platform:darwin Issues relating to macOS platform:windows Issues relating to Windows and MSVC
Projects
Development

Successfully merging this pull request may close these issues.

Populate environment for macOS builds
2 participants