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

Improve robustness of our devtools Gradle code #11303

Closed
ia3andy opened this issue Aug 10, 2020 · 14 comments · Fixed by #11517
Closed

Improve robustness of our devtools Gradle code #11303

ia3andy opened this issue Aug 10, 2020 · 14 comments · Fixed by #11517
Assignees
Labels
area/gradle Gradle kind/enhancement New feature or request
Milestone

Comments

@ia3andy
Copy link
Contributor

ia3andy commented Aug 10, 2020

Description
I was rebasing #9353 with recent fix from @gastaldi #11253, when I just stopped because it was too much for me to take in, the problem is not with @gastaldi or @mgorniew contribution which are great, our Gradle Devtool code is just too messy in its root. You can find most of it in AbstractGradleBuildFile. This is really too fragile: add a space to a build.gradle and it doesn't work anymore.

There are 3 really bad practices in this files:

  1. We do the checks from formatted String
  2. We format the snippets with some huge StringBuilders
  3. We deal with the build.gradle model as a String

I think we could easily improve 1. and 2., 3 maybe a more complex. For 1., we could use regexp and for 2. we could use some Qute snippets template.

I believe, we should wait for this to merge #9353.

@ia3andy ia3andy added the kind/enhancement New feature or request label Aug 10, 2020
@quarkusbot quarkusbot added the area/gradle Gradle label Aug 10, 2020
@quarkusbot
Copy link

/cc @quarkusio/devtools, @glefloch

@ia3andy
Copy link
Contributor Author

ia3andy commented Aug 11, 2020

@mgorniew would you like to contribute to this, it seems you have a good vision of the Gradle code?

@mgorniew
Copy link
Contributor

@ia3andy Sure, I will try to refactor/improve this code. Should I rebase #9353 first?

@ia3andy
Copy link
Contributor Author

ia3andy commented Aug 11, 2020

@mgorniew I think it could be less work to merge after the refactor, but up to you :)

@ia3andy ia3andy assigned mgorniew and unassigned gastaldi and ia3andy Aug 11, 2020
@ia3andy
Copy link
Contributor Author

ia3andy commented Aug 11, 2020

Example to render a Qute template (data is a nested Map<String, Object>):

final String content = platformDescriptor.getTemplate("templates/gradle-dependencies-block.qute.tpl")
final Engine engine = Engine.builder().addDefaults()
                .removeStandaloneLines(true)
                .build();
final String rendered = engine.parse(content).render(data);

https://quarkus.io/guides/qute-reference

@mgorniew
Copy link
Contributor

@ia3andy I've look at code and wonder if need *Creator classes at all. They are currently used only used by CreateProjectCommandHandler to add additional dependencies during project bootstrapping. There is already "TODO" comment there to move this to project generation templates. So I would like to remove those classes entirely and for gradle add additional dependencies in ftl templates. Not sure about maven. I can leave ExtensionManager calls or modify ftl for pom.xml file also. What do you think about this?

@ia3andy
Copy link
Contributor Author

ia3andy commented Aug 13, 2020

I started a discussion on zulip to explain..

@ia3andy
Copy link
Contributor Author

ia3andy commented Aug 18, 2020

@mgorniew what the status of this? did you find a way to get the "bom" dependencies listed from the Gradle plugin?

@mgorniew
Copy link
Contributor

@ia3andy Only supported way which I found is to use instance of Project class which is injected to Gradle Tasks. But we can't create instances of this class outside Gradle. So current unit tests would not work any more and we will only have integration tests.

@ia3andy
Copy link
Contributor Author

ia3andy commented Aug 18, 2020

Well, if you look at AddGradleExtensionsTest.TestingGradleBuildFile I guess that would solve your problem right?

@ia3andy
Copy link
Contributor Author

ia3andy commented Aug 18, 2020

(maybe you would need to improve it a bit obviously)

@mgorniew
Copy link
Contributor

@ia3andy I could do that, but doesn't this defies the purpose of GroovyBuildFileTest and KotlinBuildFileTest? Those classes basically tests ConnectorDependencyResolver.getDependencies() method (which use GradleConnector). If I replace getDependencies with code from AddGradleExtensionsTest.TestingGradleBuildFile then we will test test code. Overall I don't see anything wrong with using Project class and test this in integration-tests - we will just catch potential errors later during build.

@ia3andy
Copy link
Contributor Author

ia3andy commented Aug 18, 2020

@mgorniew I am ok with that! But I would still add keep/some the unit tests to test the logic that we have in the common (grovvy/kotlin dsl formatting...). wdyt?

@aloubyansky
Copy link
Member

@mgorniew i haven't followed the discussion on this one. But if you need access to Project, perhaps, you could look into using custom models. Have you seen how we load QuarkusModel?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/gradle Gradle kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants