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

use 'plugins' instead of 'apply plugin' in gradle-it #1926

Closed

Conversation

vorburger
Copy link
Contributor

@vorburger vorburger commented Apr 8, 2019

related to #1623

just for consistency with the Quick Start and our doc on https://quarkus.io/guides/gradle-tooling and https://quarkus.io/guides/gradle-config.html

FTR: This change does not actually fix the currently broken Gradle IT.

@vorburger
Copy link
Contributor Author

@jnizet better like this, right? Do you happen to have an idea about the TODO ?

@stalep @geoand @Sanne @aloubyansky @dmlloyd

@Sanne
Copy link
Member

Sanne commented Apr 8, 2019

well that TODO is exactly the reason for which I'd not use the IDs :)

I'd agree this is the way people should be using a tagger release once we publish them on the Gradle plugin portal, but I don't think we can benefit from the nicer syntax within our strange build.

@jnizet
Copy link
Contributor

jnizet commented Apr 8, 2019

I don't have a clear idea.

AFAIK, a typical gradle plugin test would use the Gradle test kit (see https://docs.gradle.org/current/userguide/test_kit.html#sub:test-kit-automatic-classpath-injection).

In this case, if it's not possible to publish the plugin in mavenLocal before executing this integration test, I guess applying the plugin imperatively is indeed the easiest solution.

@vorburger
Copy link
Contributor Author

if it's not possible to publish the plugin in mavenLocal before executing this integration test

@jnizet So while I am thinking this through more, technically the built plugin IS of course actually already published into mavenLocal in quarkus/devtools/gradle by the POM magic of maven-resources-plugin which copies the Plugin JAR built by Gradle in build/libs/ into target/ and then uses build-helper-maven-plugin to attach-artifact - so a regular ./mvnw clean install build does actually put quarkus-gradle-plugin-999-SNAPSHOT.jar into e.g. ~/.m2/repository.

well that TODO is exactly the reason ...

@Sanne Why is just using mavenLocal() (and just removing the TODO ..) actually all that bad?

@Sanne
Copy link
Member

Sanne commented Apr 8, 2019

@Sanne Why is just using mavenLocal() (and just removing the TODO ..) actually all that bad?

it's based on several assumptions which might not hold:

  • that you're actually using "install" from Maven
  • that you're installing into the same repo
  • that the most recent installed snapshot is actually belonging to this build
  • that you're not having parallel builds running (I do often!)

Sure they aren't very likely, but also I don't see why not keeping it as is as it's slightly safer. Sure it looks horrible but it's temporary :)
As mentioned, I agree that consumers should use the plugin via its id, but how we run these tests doesn't really matter.

I wouldn't bother too much with this either. As soon as build related tasks "calm down" and some relatively stable APIs emerge, I suggest we move the gradle related bits to a fully Gradle based project (but wouldn't move it yet: too many things changing too fast ;-) )

One thing we could do already is to merge the two modules: the Gradle build plugin and its integration tests could totally coexist in the same module, and use the Gradle test kit. I was going to try doing that but didn't have time yet - I know @stalep agrees as well. If you're familiar with Gradle maybe do that instead?

@vorburger
Copy link
Contributor Author

it's based on several assumptions which might not hold:
that you're actually using "install" from Maven

I do know what you mean, but isn't everyone using Maven resigned to mvn clean install, always? 😸 If you have that sort of ambition, then this entire project should just use a modern build tool like Gradle or even better Bazel instead of Maven all together... 😈

More seriously, if Quarkus currently actually builds fine with just mvn package then yeah we should not require install just for the Gradle plugin and its IT - agreed.

that you're installing into the same repo
that the most recent installed snapshot is actually belonging to this build
that you're not having parallel builds running (I do often!)

There is always -Dmaven.repo.local=, but yes not sure the build.gradle would pick that up.

we move the gradle related bits to a fully Gradle based project

I would rather go step by step and focus on having finishing up v1.0 of #1623, first.

coexist in the same module, and use the Gradle test kit

I've no idea how to do that. In an overall Maven based project like this, it actually seems better to me not to rely on pure Gradle world IT - ultimately what this produces, from Maven, has to work with a "standalone" Gradle project. (I'm even wondering if the Quick Start couldn't be used for an IT, but that's perhaps for later.)

So #1954 is a (first step of) doing it differently - using a <useRepositoryLayout>true makes it closer to reality and lets use maven { url 'target/dependencies/' }. If you like and merge that, I'll change this PR to be based on that approach (and then #1929).

vorburger and others added 4 commits April 10, 2019 15:34
related to quarkusio#1623

just for consistency with the Quick Start and our doc on
https://quarkus.io/guides/gradle-tooling and
https://quarkus.io/guides/gradle-config.html

FTR: This change does not actually fix the currently broken Gradle IT.
reproduced locally after wiping:
~/.m2/repository/javax/enterprise/cdi-api/
 ~/.m2/repository/commons-logging
~/.m2/repository/org/jboss/xnio
see details in discussion of PR
@vorburger vorburger force-pushed the issue-1623_gradle-it_plugins branch from 45e02c5 to 7b10013 Compare April 10, 2019 14:52
@vorburger
Copy link
Contributor Author

With @stalep having merged #1954 (thanks!), I've changed this PR to be based on that approach (it now does not rely on mavenLocal, anymore). @stalep @Sanne if you like and merge this, I'll change then adapt #1929 accordingly - and THEN contribute #1623!

vorburger added a commit to vorburger/quarkus that referenced this pull request Apr 10, 2019
vorburger added a commit to vorburger/quarkus that referenced this pull request Apr 10, 2019
@vorburger
Copy link
Contributor Author

@stalep @Sanne would you be OK to merge this now, and then #1929?

Please just shout if you want a rebase or squash, or something.

@stalep
Copy link
Member

stalep commented Apr 11, 2019

I was wondering if it would be better to merge #1929 and #1986 and this into one pr/commit or will that overlap too much?

@stalep
Copy link
Member

stalep commented Apr 11, 2019

and #1985

vorburger added a commit to vorburger/quarkus that referenced this pull request Apr 11, 2019
use 'plugins' instead of 'apply plugin' in gradle-it
but avoid using mavenLocal() in devtools/gradle-it (see
details for why in quarkusio#1926)

related to quarkusio#1623

and for consistency with the Quick Start and our doc on
https://quarkus.io/guides/gradle-tooling and
https://quarkus.io/guides/gradle-config.html
@vorburger
Copy link
Contributor Author

@stalep closing this, as this is now squashed into #1929. (Let's deal with #1985 and #1986 separately.)

@vorburger vorburger closed this Apr 11, 2019
@stalep
Copy link
Member

stalep commented Apr 11, 2019

closing it as #1929 is doing the same and more

@gsmet gsmet added the triage/out-of-date This issue/PR is no longer valid or relevant label Apr 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage/out-of-date This issue/PR is no longer valid or relevant
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants