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

Fix NdkToolchain.baseDir configuration cache #491

Closed
wants to merge 1 commit into from

Conversation

ZacSweers
Copy link
Contributor

This property should be a DirectoryProperty rather than Property<File> in order to be compatible with configuration cache. This also allows for it to be more chainable with providers of this from android components.

Goal

See above

Design

See above

Changeset

See above

Testing

Manually tested in our project via maven local.

This property should be a `DirectoryProperty` rather than `Property<File>` in order to be compatible with configuration cache. This also allows for it to be more chainable with providers of this from android components.
@ZacSweers
Copy link
Contributor Author

Testing this reveals a separate issue though, which is that it's a required property for NdkToolchain and tasks to be configured, even if they aren't run. This presents a problem for consumers that don't have the NDK installed (even if they don't use it), as the property configuration then fails. There is a flag for this via the upload task control, but it's checked eagerly and hard to disable. I could take a crack at improving it here (i.e. make the baseDir property optional), or leave that for a followup.

@johnkiely1
Copy link
Member

Thanks for the PR @ZacSweers, we will review as soon as we can.

@johnkiely1 johnkiely1 added bug Confirmed bug backlog We hope to fix this feature/bug in the future labels Jan 20, 2023
@lemnik lemnik self-requested a review January 20, 2023 10:09
@lemnik
Copy link
Contributor

lemnik commented Jan 20, 2023

Hi @ZacSweers, thanks so much for this PR and pointing out the eager-config issue as well. Would you suggest making the NdkToolchain references a Provider instead of a hard-reference (specifically for tasks register functions)?

@ZacSweers
Copy link
Contributor Author

ZacSweers commented Jan 20, 2023

I'm not sure if that would help. I think where you'd want to end up is probably somewhere where you register the tasks regardless but make them no-op (± a warning log) or error at task-action-time if no NDK is found

@lemnik
Copy link
Contributor

lemnik commented Feb 14, 2023

@ZacSweers I've moved this commit over to: #499 so I'm closing this PR.

@lemnik lemnik closed this Feb 14, 2023
@ZacSweers ZacSweers deleted the z/ndkBaseDirFix branch February 14, 2023 16:11
@lemnik lemnik mentioned this pull request Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog We hope to fix this feature/bug in the future bug Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants