[native_assets_cli] Introduce CCompilerConfig.windows
#1913
Conversation
This file contains 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
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
|
mosuem
approved these changes
Jan 21, 2025
|
|
||
| /// The json representation of this [CCompilerConfig]. | ||
| /// | ||
| /// The returned json can be used in [CCompilerConfig.fromJson] to | ||
| /// obtain a [CCompilerConfig] again. | ||
| Map<String, Object> toJson() => { | ||
| Map<String, Object> toJson({bool deprecatedTopLevel = false}) => { |
There was a problem hiding this comment.
Documentation on this parameter is missing
|
|
||
| /// Configuration provided when [CodeConfig.targetOS] is [OS.windows]. | ||
| final class WindowsCCompilerConfig { | ||
| final DeveloperCommandPrompt? developerCommandPrompt; |
There was a problem hiding this comment.
FWIW, I don't think the name is too long
| /// The Windows Developer Command Prompt. | ||
| /// | ||
| /// Sets up the environment variables for [CCompilerConfig.compiler], | ||
| /// [CCompilerConfig.linker], and [CCompilerConfig.archiver]. |
There was a problem hiding this comment.
Suggested change
| /// [CCompilerConfig.linker], and [CCompilerConfig.archiver]. | |
| /// [CCompilerConfig.linker], and [CCompilerConfig.archiver] on Windows. |
auto-submit bot
pushed a commit
that referenced
this pull request
Mar 26, 2025
Closes: #1826 This PR adds the final step to the generated syntax validation: conditionally required fields: 1. If target OS is `x`, then require `x` config in code config. 2. If target OS is `windows`, then require `windows` in the c compiler config (more info #1913). 3. If link mode is dynamic library bundled or static library, then require a file in a code asset. We could consider trying to nest the fields under the condition, but that has other downsides: RE 1: Then the OS is no longer an enum usable in the code-asset as OS field. (We could consider this if we remove the OS/arch from code asset outputs. We should be able to do this due to the code config always having a single OS and architecture anyway. #2127) RE 2: That would mean the compiler config would be split over two places. `input.config.code.cCompiler` and `inputconfig.code.windows.cCompiler`. Maybe that's better? Maybe not? RE 3: Treating a group of files in assets would then become `input.assets.code.switch( ... )` instead of simply `input.assets.code.map((a) => a.file)`. Maybe that's okay because we don't often use files in such way anyway? WDYT @mosuem @HosseinYousefi? (I'd probably do any of those refactorings in follow up PRs.)
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: #1606
This PR moves
cCompilerConfig.envScripttocCompilerConfig.windows.developerCommandPrompt.script.Notable changes:
DeveloperCommandPromptis optional, but if it's set, both the script and arguments must be set. Since clang on Windows detects the MSVC installations, thedeveloperCommandPromptitself is optional for the case where clang is passed as the compiler on WindowsCodeConfighave been renamed to<OS>CodeConfigavoid naming conflicts with<OS>CCompilerConfig.Up for discussion:
developerCommandPromptis rather long)Future<Map<String, String>> loadEnvironment()onDeveloperCommandPrompt? It's a bit out of place because the config is rather only a view on the JSON. However, it is most likely use case.