Skip to content

[native_assets_cli] Nest c_compiler config #30

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

Merged
merged 6 commits into from
May 1, 2023
Merged

Conversation

dcharkes
Copy link
Collaborator

@dcharkes dcharkes commented May 1, 2023

Changes the format for the YAML/JSON BuildConfig to have a nested c_compiler key. Example config:

c_compiler:
  ar: /tmp/MLTHBB/fake_ar
  cc: /tmp/MLTHBB/fake_clang
  env_script: /tmp/MLTHBB/fake_env_script.bat
  env_script_args: []
  ld: /tmp/MLTHBB/fake_ld
dependency_metadata:
  bar:
    key: value
  foo:
    a: 321
    z:
      - z
      - a
link_mode_preference: prefer-static
out_dir: /tmp/MLTHBB/out1/
package_root: /tmp/MLTHBB/
target: ios_arm64
target_ios_sdk: iphoneos
version: 1.0.0

This changes the environment variables to set if the compiler cannot be auto-detected on PATH or default install locations to:

  • C_COMPILER__AR
  • C_COMPILER__CC
  • C_COMPILER__LD
  • C_COMPILER__ENV_SCRIPT
  • C_COMPILER__ENV_SCRIPT_ARGS

These will need to be updated in the Dart SDK: https://dart-review.googlesource.com/c/sdk/+/300221.

For reference internal design doc for the protocol.

Closes: #12

@coveralls
Copy link

coveralls commented May 1, 2023

Coverage Status

coverage: 99.846% (+0.003%) from 99.843%
when pulling d71c2c1 on build-config-hierarchy
into e5cac81 on main.

@dcharkes dcharkes requested a review from whesse May 1, 2023 10:41
@whesse
Copy link

whesse commented May 1, 2023

I see the names of the environment variables have not been changed in the internal design doc. I assume you have buy-in from stakeholders on these new names.

I had to do a bunch of digging to see how the new environment values were being fetched in the non-test case: in package cli_config which is in the tools repository, with environment parser converting the env variable name to the key value.

@dcharkes
Copy link
Collaborator Author

dcharkes commented May 1, 2023

Thanks @whesse! 🙏

I see the names of the environment variables have not been changed in the internal design doc. I assume you have buy-in from stakeholders on these new names.

This is addressing Martin's feedback on an earlier PR.

For now, we don't have to settle on the final API yet. That point comes when we start publishing these packages and stop using an experimental flag for the dartdev support. (Which at this point has not landed yet.)

I had to do a bunch of digging to see how the new environment values were being fetched in the non-test case: in package cli_config which is in the tools repository, with environment parser converting the env variable name to the key value.

Correct! 👌

(That was also why the non-test-case was/is failing on https://dart-review.googlesource.com/c/sdk/+/267340, I used a different env key for the script and script args.)

@dcharkes dcharkes marked this pull request as ready for review May 1, 2023 11:56
@dcharkes dcharkes merged commit a720376 into main May 1, 2023
@dcharkes dcharkes deleted the build-config-hierarchy branch May 1, 2023 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[native_assets_cli] BuildConfig BuildOutput hierarchy
3 participants