[native_assets_builder] Don't pass in the whole environment #1764
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dcharkes
changed the title
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
|
dcharkes
force-pushed
the
allow-list-env-vars
branch
from
4481101 to
189e7da
Compare
dcharkes
force-pushed
the
allow-list-env-vars
branch
from
189e7da to
31b3ecf
Compare
dcharkes
force-pushed
the
allow-list-env-vars
branch
from
31b3ecf to
22d51b8
Compare
|
@mkustermann This PR now shows only the diff to the PR it is stacked on top of. |
mkustermann
approved these changes
Dec 3, 2024
pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart
Outdated
Show resolved
Hide resolved
auto-submit bot
pushed a commit
that referenced
this pull request
Dec 4, 2024
…ge (#1759) This PR makes the native assets builder save the `Platform.environment` a hook is run in. Subsequent invocations check if the environment didn't change. This removes the `includeParentEnvironment` parameter everywhere. It was always set to true in `dartdev` and `flutter_tools` (This will require a manual roll into the Dart SDK and Flutter tools.) A follow up PR should restrict the list of environment variables (#1764), this PR is about caching correctness. Bug: * #32
github-merge-queue bot
pushed a commit
to flutter/flutter
that referenced
this pull request
Jan 2, 2025
This PR rolls in a number of breaking changes from dart-lang/native: * `BuildMode` is no longer part of the protocol, so Flutter no longer passes it in. * This means all code dealing with the name conflict between `native_assets_cli.BuildMode` and `flutter_tools.BuildMode` has been cleaned up. * Also, the logs no longer mention the build mode. * The tests still exercise both modes, because linking only happens in release mode. * `OS` is no longer part of the main protocol, but of the "code" "protocol extension". * The code now aligns more with `OS?` being nullable in a bunch of places, since it is nullable if there's no code assets. * The OS-specific config is nested in an object per OS. * `CCompilerConfig`s fields are non-nullable now. * So instead of passing an object with nullable fields around, a null instead of the object is returned in various places. * `FileSystem` is now passed in to the native assets builder. This PR contains no feature changes. This PR will need to be followed up by restricting what environment variables are passed in (similar to dart-lang/native#1764), I will do this in a follow up PR. Tests: * All existing features should be covered by existing tests.
|
This probably should be the opposite, where env variables are allowed except for known conflicts. If it's a minimal list, many unknown toolchains can be broken. It seems this issue with SDKROOT stems from dart-lang/sdk@bd2452c#diff-728a6a243e6aa631a402856e897c7c1e5585ad206e98674fb18fcfee2d4ef224R265 I think removing the ENV variables in that commit on Mac would only break python for child build processes? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: #32
See the referenced issue for a reasoning on the list of environment variables.
Stacked on top of: