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

feat(manager/gradle): add support for apply from statements #16030

Merged
merged 24 commits into from
Jul 3, 2022

Conversation

Churro
Copy link
Collaborator

@Churro Churro commented Jun 12, 2022

Changes

  • Adds support for apply from: 'somefile.gradle' statements
    • Enables dynamic inclusion of further, arbitrary files during the parsing operation of a gradle file. vars, deps and urls of included files are then evaluated in the context of the file that is currently parsed.
    • Implements parts of the corresponding Groovy DSL and Kotlin DSL.

No existing tests were changed/rewritten, therefore I don't expect any breaking changes.

Test repository: https://github.com/Churro/renovate-applyfrom/pulls

Implementation Notes

  • extract.ts:

    • Files included via apply from do not necessarily have to be *.gradle, so they may not exist in packageFiles and are dynamically added, if missing.
    • If files are of type *.gradle, it may happen that they are parsed twice. It's ensured that extracted deps are still considered only once.
  • parser.ts:

    • It's anachronistic to introduce readLocalFileSync() but I really don't see a reasonable way how to do this async.
      • The currently parsed file needs the included file the moment when apply from happens, not afterwards.
      • Parsing one or multiple included files ahead would probably incur a lot more complexity since you would need to go over all packageFiles at least twice and then selectively merge parsed infos. This step would need to be done in any case, also if no apply from is used in any file.
    • Files could, in theory, include themselves, which would lead to an endless loop. A recursionDepth check ensures this cannot happen, as it prevents "nested apply from" statements. If there is good reason, this could be increased to 1, 2, etc. and would still do no harm, but in any case the check remains a threshold to prevent cyclic includes.

Known Limitations

Context

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please tick one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

lib/modules/manager/gradle/parser.ts Outdated Show resolved Hide resolved
@viceice viceice requested a review from zharinov June 12, 2022 23:38
@rarkins rarkins requested a review from viceice June 15, 2022 10:10
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment, i've prepared a commit to use async io (5min of work)

bdbc7c8

lib/util/fs/index.ts Outdated Show resolved Hide resolved
# Conflicts:
#	lib/modules/manager/gradle/extract.spec.ts
#	lib/modules/manager/gradle/extract.ts
#	lib/modules/manager/gradle/parser.spec.ts
@Churro Churro dismissed a stale review via 7998581 June 25, 2022 18:48
@rarkins rarkins requested a review from viceice June 27, 2022 05:26
@rarkins rarkins marked this pull request as ready for review June 27, 2022 05:26
zharinov
zharinov previously approved these changes Jun 27, 2022
Copy link
Collaborator

@zharinov zharinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm convinced by your reasoning and the implementation, looks good to me 👍

lib/modules/manager/gradle/extract.ts Outdated Show resolved Hide resolved
lib/modules/manager/gradle/extract.spec.ts Outdated Show resolved Hide resolved
lib/modules/manager/gradle/extract.spec.ts Outdated Show resolved Hide resolved
lib/modules/manager/gradle/extract.spec.ts Outdated Show resolved Hide resolved
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see @zharinov comments

Co-authored-by: Sergei Zharinov <zharinov@users.noreply.github.com>
Churro and others added 2 commits June 27, 2022 22:42
Co-authored-by: Sergei Zharinov <zharinov@users.noreply.github.com>
Co-authored-by: Sergei Zharinov <zharinov@users.noreply.github.com>
@rarkins rarkins requested review from zharinov and viceice June 28, 2022 03:49
@Churro Churro requested a review from viceice June 28, 2022 19:15
lib/modules/manager/gradle/extract.spec.ts Show resolved Hide resolved
lib/util/fs/index.spec.ts Outdated Show resolved Hide resolved
lib/modules/manager/gradle/extract.ts Outdated Show resolved Hide resolved
Churro and others added 2 commits June 29, 2022 10:18
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
@Churro Churro requested a review from viceice June 29, 2022 17:29
lib/modules/manager/gradle/extract.spec.ts Outdated Show resolved Hide resolved
lib/modules/manager/gradle/extract.ts Outdated Show resolved Hide resolved
lib/modules/manager/gradle/parser.ts Outdated Show resolved Hide resolved
lib/modules/manager/gradle/parser.ts Show resolved Hide resolved
lib/modules/manager/gradle/parser.ts Show resolved Hide resolved
@viceice viceice merged commit d19d645 into renovatebot:main Jul 3, 2022
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 32.104.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants