-
Notifications
You must be signed in to change notification settings - Fork 68
[native_assets_builder] add WINDIR
and SYSTEMDRIVE
to allowed env list
#2084
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
Conversation
…nmentVariablesFilter`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm w comments
@@ -433,6 +433,8 @@ ${e.message} | |||
'TMP', // Needed for temp dirs in Dart process. | |||
'TMPDIR', // Needed for temp dirs in Dart process. | |||
'USER_PROFILE', // Needed to find tools in default install locations. | |||
'WINDIR', // Needed for CMake. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Alphabetical sorting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean to sort the added 2 items or the whole set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole set 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 😄
@@ -433,6 +433,8 @@ ${e.message} | |||
'TMP', // Needed for temp dirs in Dart process. | |||
'TMPDIR', // Needed for temp dirs in Dart process. | |||
'USER_PROFILE', // Needed to find tools in default install locations. | |||
'WINDIR', // Needed for CMake. | |||
'SYSTEMDRIVE', // Needed for CMake. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for the CMAKE_TOOLCHAIN_FILE
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's not necessary, and usually CMAKE_TOOLCHAIN_FILE
is not an env variable but a definition of cmake cli.
PR HealthBreaking changes ✔️
Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
License Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
|
Closes: #2077
As @dcharkes suggested, this PR add
WINDIR
andSYSTEMDRIVE
tohookEnvironmentVariablesFilter
atnative/pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart
Lines 426 to 436 in 318f841
Contribution guidelines:
dart format
.Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.