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 gradlew location when trying to location a gradle executable #610

Merged
merged 2 commits into from
Jan 11, 2023

Conversation

LouisBoudreau
Copy link
Contributor

The #606 fix changed the search location of the gradlew executable
According to the documentation the gradlew should be present at the root of the project :

A Gradle project typically provides a settings.gradle file and one build.gradle file for each subproject. The Wrapper files live alongside in the gradle directory and the root directory of the project.

This PR therefore aims at updating where licensed searches for gradlew

@@ -66,7 +66,7 @@ def executable
return @executable if defined?(@executable)

@executable = begin
gradlew = File.join(config.pwd, "gradlew")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For gradle multi-projects builds, the config.pwd is often a subfolder and not the root of the project.
I think licensed should look for gradlew at the root since it is the official location for the gradle wrapper

What do you think about that ?

@jonabc
Copy link
Contributor

jonabc commented Jan 10, 2023

The #606 fix changed the search location of the gradlew executable

Could you explain this a bit more? The config.pwd value used when finding the executable is the same value that was passed as the root path into Gradle::Runner.new. I don't see why/how the path would have changed with that PR.

Otherwise, I want to caution any expectation that config.root is equivalent to project root. If not explicitly configured, it's most likely going to be the git repository root. For monorepos which have multiple languages or projects within the same repo, it's common to have a single licensed configuration at the root of the repo with many apps for each of the many projects in the monorepo.

What do you think about adding a gradle.gradlew config setting to path off of config.root rather than expecting that config.root points at any specific directory? It could default to assuming that config.root is the same folder as settings.gradle if that's going to be the most common arrangement, which is what I think you've implemented here. Then users can change configure where to find gradlew if config.root points at any other location.

e.g. with the extra config value available there's quite a bit of flexibility. I don't know of any monorepos that mix gradle and go but who knows? there's probably something out there 😆

# in .licensed.yml config file

root: ../some/other/root/path

apps:
  - source_path: 'path/from/root/to/gradle/lib/*'
     sources:
       gradle: true
  - source_path: 'path/from/root/to/go/cmd/*'
     sources:
       go: true

gradle:
  gradlew: 'path/from/root/to/gradle/gradlew'

Hopefully that makes sense!

@LouisBoudreau
Copy link
Contributor Author

I don't see why/how the path would have changed with that PR.

After re-reading the changes and the command class source code, I confirm that I effectively made a false assumption that the change broke the search location for gradlew. Im sorry for this. What happened is that somewhere during the for the local development of #583 I installed gradle and forgot to delete it to make the final tests for our multi-project build. licensed was therefore using the gradle executable instead of the gradlew from our project.

Your idea to add a new property is really good! I implemented it, let me know if there is anything else I can do 😄

Copy link
Contributor

@jonabc jonabc left a comment

Choose a reason for hiding this comment

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

🚀

Comment on lines +26 to 33
### Gradlew

The `gradle.gradlew` property is used to determine where the `gradlew` executable is. The default location the [configuration root](../configuration/configuration_root.md).

```yml
gradle:
- gradlew: path/from/root/to/gradle/gradlew
```
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️ thanks for the docs update!!

Copy link
Contributor

Choose a reason for hiding this comment

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

oh 🤦 the nested gradlew property isn't an array and shouldn't have the - prefix. I'll fix the docs in the release PR

@jonabc jonabc merged commit ca27420 into github:master Jan 11, 2023
@LouisBoudreau LouisBoudreau deleted the fix-gradle-wrapper-location branch January 11, 2023 16:43
jonabc added a commit that referenced this pull request Jan 11, 2023
## 4.0.2

### Fixed
- The path to a gradlew executable can be configured when enumerating gradle dependencies (:tada: @LouisBoudreau #610)
@jonabc jonabc mentioned this pull request Jan 11, 2023
jonabc added a commit that referenced this pull request Jan 11, 2023
- The path to a gradlew executable can be configured when enumerating gradle dependencies (:tada: @LouisBoudreau #610)
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