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

Automatically detect correct build profile (no more -Pcomplete) #403

Merged
merged 2 commits into from
Oct 13, 2023

Conversation

holly-cummins
Copy link
Collaborator

Resolves #233
Resolves #378

I think the reason we originally had profiles is that depending what zip they extract, sometimes people have lots of modules, sometimes they have a small number, and maven does not allow us to define optional modules, or just do a “build all subfolders” command.

The mvn docs say:

All profile elements in a POM from active profiles overwrite the global elements with the same name of the POM or extend those in case of collections. In case multiple profiles are active in the same POM or external file, the ones which are defined later take precedence over the ones defined earlier (independent of their profile id and activation order).

So anything we define in a profile will add to anything we define in the main pom, but profiles won’t add to each other, and we need to define them in order.

We can only define one ‘guard’ file per profile. The good news is that limits the combinatorics, and we have to assume a limited number of ‘profiles’ (as it were!) to cater for. This does mean if someone is midway through building the workshop and tries a top-level build, it will fail because of missing projects or, depending what guard file we choose, not build everything.

This is annoying, but no worse than where we were before. At least builds of the extracted zips will work.

Well, they would build if it wasn’t for #280. I’ll fix that next, and also add some tests to make sure the zips can actually build, since it seems like core technical capability.

@holly-cummins
Copy link
Collaborator Author

It turns out there are some subtleties with reactors build order and profiles. The reason some of the instructions and readmes build the extension module first is that, under some circumstances, maven will build the deployment module of the extension quite late. It doesn't take transitive dependencies into account when deciding the reactor order. It seemed like one way of defining the profiles got the correct order, so it behaved like the old -Pcomplete, and the other got an incorrect order and was broken. So I've gone with the way that works, but it's a bit more fragile than is ideal.

@holly-cummins
Copy link
Collaborator Author

Another complexity - we used to only build native for the core profile in the CI, but with this change, we build it for everything. That seems like the right kind of check to be doing in the CI, but it's

(a) slow
(b) failing

I've gone back to just building the core with native, and raised #404 and #405.

@holly-cummins holly-cummins force-pushed the no-manual-profiles branch 2 times, most recently from 7c2f6b9 to 3734972 Compare October 13, 2023 10:15
@agoncal
Copy link
Collaborator

agoncal commented Oct 13, 2023

I remember that @cescoffier had to refactor the directory structure and change the pom to make sure the extension was built first, and then the app in second

Copy link
Collaborator

@agoncal agoncal left a comment

Choose a reason for hiding this comment

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

OMG I can already see some merge conflicts when I'll create my PR ;o)

@github-actions
Copy link

github-actions bot commented Oct 13, 2023

🙈 The PR is closed and the preview is expired.

@holly-cummins holly-cummins merged commit 55e6ec6 into quarkusio:main Oct 13, 2023
6 checks passed
@holly-cummins
Copy link
Collaborator Author

Merging, @agoncal! The exclude for the extension version build isn't working - maybe it needs to be extension-version-parent? - but I will do that in a follow-up PR.

@agoncal
Copy link
Collaborator

agoncal commented Oct 13, 2023

Ok. I'll be careful with the extension then

@holly-cummins
Copy link
Collaborator Author

Ok. I'll be careful with the extension then

I think there's no need, just be aware that the CI build is annoyingly long now (doh, sorry!), and one mitigation that looks like it's there (not doing a native build of the extension) isn't actually working. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants