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

Remove use/mention of 'compile quarkus:dev' #28999

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

maxandersen
Copy link
Member

compile quarkus:dev was needed in pre 1.0 Quarkus - time to remove it as not needed.

@quarkus-bot quarkus-bot bot added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/elasticsearch area/gradle Gradle area/kotlin area/maven area/platform Issues related to definition and interaction with Quarkus Platform area/scala labels Nov 2, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Nov 2, 2022

/cc @gsmet, @loicmathieu, @yrodiere

@quarkus-bot
Copy link

quarkus-bot bot commented Nov 2, 2022

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

@maxandersen maxandersen changed the title remove use/mention of 'compile quarkus:dev' Remove use/mention of 'compile quarkus:dev' Nov 2, 2022
@aloubyansky
Copy link
Member

We have to be careful about this. Explicit compile and compile invoked by the DevMojo don't seem to always work in the same way. For single module projects, autocompile will work most of the time, probably.
However, for multi module projects we should be recommending explicit compile when the launch is performed from the root project dir.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

This looks good to me but it will require a squash.

I will let @aloubyansky decide if it's the way to go though. My understanding is that we already recommended using mvn quarkus:dev.

@quarkus-bot

This comment has been minimized.

@@ -861,7 +861,7 @@ Run the application and notice the `Installed Features` list contains the `greet

[source,shell,subs=attributes+]
----
$ mvn clean compile quarkus:dev
Copy link
Member

Choose a reason for hiding this comment

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

Kind of minor but the logs below are for mvn clean compile quarkus:dev.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated it - interesting to see how more "noisy" our output become in a year ;)

@@ -2278,7 +2278,7 @@ Otherwise, Quarkus attempts to monitor the extension classes and this may result

[source,bash]
----
./mvnw compile quarkus:dev -DnoDeps
Copy link
Member

Choose a reason for hiding this comment

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

This paragraph is dedicated to multimodule projects and this is where we should mention when explicit compile is recommended. This paragraph is also outdated. Our bootstrap has evolved since those days and is a lot more reliable today. -DnoDeps isn't really required today.

Copy link
Member

Choose a reason for hiding this comment

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

+1 on mentioning compile, maybe even test-compile for continuous testing.

PS: Because of a error when using ECJ, I had to resort to specifying an explicit goal, so that devmojo doesn't run those executions on its own (because this is where the error happens).

Copy link
Member

Choose a reason for hiding this comment

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

@aloubyansky I think it would probably be easier if you added an additional commit on top of @maxandersen 's changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

@famod what's that error you get with each? Got issue link?

@gsmet sorry. GitHub notifications sucks for me. Will touch this up while travelling today.

Copy link
Member Author

Choose a reason for hiding this comment

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

@aloubyansky I've reworked this section to talk about need for explicit compile and removed noDeps mention. Should it be mentioned somewhere else or is noDeps just not relevant to use anymore ?

Copy link
Member

Choose a reason for hiding this comment

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

By default we make as many modules in the workspace reloadable as we can handle (exceptions are extension artifacts, dependencies of extensions, parent-first artifacts and dependencies of parent-first).
noDeps makes only the app root module reloadable.

@maxandersen
Copy link
Member Author

We have to be careful about this. Explicit compile and compile invoked by the DevMojo don't seem to always work in the same way.

"don't seem to" as in the behaviour looks differently to users and thus must use compile for consistency or "don't seem to" as in we don't know why compile is needed but it fixes the issue?

@quarkus-bot

This comment has been minimized.

@gsmet
Copy link
Member

gsmet commented Feb 1, 2023

Could we fix the issues and finalize this? @aloubyansky I think you're the best person to get this done quickly.

@gsmet gsmet added the triage/needs-feedback We are waiting for feedback. label Feb 1, 2023
@aloubyansky
Copy link
Member

@gsmet I think it's ok. It does now mention compile is recommended for multimodule projects.

@rsvoboda
Copy link
Member

rsvoboda commented Feb 2, 2023

@maxandersen PR needs update, one conflicting file

@gsmet gsmet added the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Feb 4, 2023
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I rebased the PR, it can now be merged once CI is green.

@gsmet gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 22, 2024
@gsmet gsmet dismissed rsvoboda’s stale review October 22, 2024 16:36

Comments were addressed.

Copy link

github-actions bot commented Oct 22, 2024

🙈 The PR is closed and the preview is expired.

This comment has been minimized.

This comment has been minimized.

@gsmet gsmet merged commit 678bf8d into quarkusio:main Oct 23, 2024
3 of 8 checks passed
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 23, 2024
@quarkus-bot quarkus-bot bot added this to the 3.17 - main milestone Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/elasticsearch area/gradle Gradle area/kotlin area/maven area/platform Issues related to definition and interaction with Quarkus Platform area/scala triage/flaky-test triage/needs-feedback We are waiting for feedback. triage/needs-rebase This PR needs to be rebased first because it has merge conflicts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants