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

Add setTargets to AbstractGccCompatibleToolchain to allow resetting default toolchain targets. #3124

Merged
merged 3 commits into from
Oct 14, 2017

Conversation

JaciBrunning
Copy link
Contributor

Context

This pull request is designed to allow users to reset the default platforms assigned to Gcc and Clang toolchains. This is a new version of #3061 per @ghale 's suggestion.

setTargets(String) and setTargets(String, Action) have been added to GccCompatibleToolchain.

Contributor Checklist

  • Review Contribution Guidelines
  • Sign Gradle CLA
  • Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team
  • Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective
  • Provide unit tests (under <subproject>/src/test) to verify logic
  • Update User Guide, DSL Reference, and Javadoc for public-facing changes
  • Ensure that tests pass locally: ./gradlew <changed-subproject>:check

Gradle Core Team Checklist

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation
  • Recognize contributor in release notes

@ghale
Copy link
Member

ghale commented Oct 11, 2017

Thanks for making those changes, @JacisNonsense! Looks good.

I think there are two improvements we could make here:

  1. We might change setTargets(String) to accept multiple targets: setTargets(String...). Optionally, we could add a setTargets(List<String>, Action) that allowed one to set multiple targets, and apply the same action to each, but I don't think this is necessary as it can be accomplished via eachPlatform(Action).
  2. We should have some integration test coverage for this in GccToolChainCustomisationIntegrationTest. This would create a gcc toolchain (with x86, x64 targets), a custom toolchain that sets the targets to a custom platform and then use eachPlatform() to configure a custom compiler that sets the language to french (see for an example of how you can do this simply). Your component would target all three platforms. Then assemble and verify that the x86, x64 outputs are in english (built by gcc), and the custom platform output is in french (built by custom toolchain).

@JaciBrunning
Copy link
Contributor Author

@ghale I've added the changes mentioned, changing to setTargets(String...) and adding integration test. Integration test will have to be checked by CI since I don't have a machine available to integration test at this time

@ghale ghale merged commit 0c3e74d into gradle:master Oct 14, 2017
@ghale
Copy link
Member

ghale commented Oct 14, 2017

Thanks @JacisNonsense! I made a few minor changes to get the integration test working and merged it.

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.

2 participants