Align impsort and formatting settings in bootstrap with main project

[holly-cummins]

I noticed I was getting churn in my imports while I was working in some of the bootstrap projects ... and then I the build checks failed because the imports were in the wrong order.

On investigation, I think the problem is that the bootstrap-parent pom inherits from the jboss parent, not the Quarkus parent. That means it's missing some settings, including the impsort and formatting settings. That would be ok, except that we all have shared IDE import order settings, which match what's in quarkus-parent, and do not match the impsort defaults.

I've duplicated the parent settings into the bootstrap parent, and run a sort on the project.

[holly-cummins]

That's a confusing failure. Locally, I'm good. (Cleaning and rebuilding to see if caches are biting me.)

holly@hcummins-mac quarkus % mvn net.revelc.code:impsort-maven-plugin:1.7.0:check  -f independent-projects/bootstrap/core 
[INFO] Scanning for projects...
[INFO] 
[INFO] -----------------< io.quarkus:quarkus-bootstrap-core >------------------
[INFO] Building Quarkus - Bootstrap - Core 999-SNAPSHOT
[INFO] --------------------------------[ jar ]---------------------------------
[INFO] 
[INFO] --- impsort-maven-plugin:1.7.0:check (default-cli) @ quarkus-bootstrap-core ---
[INFO] Processed 67 files in 00:00.045 (Already Sorted: 67, Needed Sorting: 0)
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  0.391 s
[INFO] Finished at: 2022-09-22T11:15:58+01:00
[INFO] ------------------------------------------------------------------------

[holly-cummins]

I got a bit confused by the local impsort caching (which not cleaned by clean). I now think I have a working set of updates, for bootstrap parent and several other projects in bootstrap which define their own impsort config.

The downside is that updating the sourcecode to these settings changed 832 files. In the future this will reduce churn, but in the short term it's a massive churn and massive changeset, so I'm not sure how best to handle the PR. (I confirm the only change in it is my pom changes and an automated sort.)

[gsmet]

I'm not sure it's a good idea to do that for 2.14. As we will have to backport things in 2.13 for a while and we are going to have a ton of conflicts.

Another option would be to backport that to 2.13 too.

/cc @rsvoboda @mkouba @aloubyansky @geoand

[holly-cummins]

I wondered about changing the XML, but not the files. Then we could temporarily disable the enforcement on those modules so they gradually become correct as people edit them, and then flip the enforcement switch later.

The downside is that a significant proportion of low-change files may never become correct just through normal editing, and we'd have to do the extra piece of work to turn off and on the enforcement.

[geoand]

Yeah, I would prefer we do not make our lives harder with the backports - 2.13 is going to be around for what is potentially a long while.

[holly-cummins]

We could also do it incrementally by project, so we could do extensions-maven-plugin now, since that's the one which is causing me pain, and then gradually work through the others so the diffs stay manageable.

[gsmet]

As I mentioned, one option would be to also backport this to 2.13. I would expect it to be safe. But I would prefer having @rsvoboda (for QE) , @mkouba and @aloubyansky (because it's mostly their code) 's blessing before doing that.

But for me it's either:

• my proposal to backport the formatting change to 2.13
• doing it for the next long term maintained version, not the one just after it
• not doing it at all (but I can understand how frustrating it could be to have your IDE enforcing something and the formatter another thing)

[holly-cummins]

As another option, could we turn off the enforcement on the affected projects in the 2.13 stream? That would mean we won't be fighting the import sorting in 2.13, which we would be otherwise, with or without this change. (Anytime someone edits one of the affected files in the IDE, they have to undo the IDE's changes to the Java file.)

[mkouba]

As I mentioned, one option would be to also backport this to 2.13. I would expect it to be safe. But I would prefer having @rsvoboda (for QE) , @mkouba and @aloubyansky (because it's mostly their code) 's blessing before doing that.

But for me it's either:

• my proposal to backport the formatting change to 2.13

+1

Let's do this now...

• doing it for the next long term maintained version, not the one just after it

• not doing it at all (but I can understand how frustrating it could be to have your IDE enforcing something and the formatter another thing)

[gsmet]

As another option, could we turn off the enforcement on the affected projects in the 2.13 stream?

No. I don't want 2.13 to turn into a pile of crap with commits changing imports and then back and so on. We are enforcing all this for a good reason.

[holly-cummins]

As another option, could we turn off the enforcement on the affected projects in the 2.13 stream?

No. I don't want 2.13 to turn into a pile of crap with commits changing imports and then back and so on. We are enforcing all this for a good reason.

Yes, after I thought about it I realised disabling on 2.13 might be almost as much risk as enforcing the correct rules, without any of the corresponding benefit.

There's a larger question about whether we want a common parent for those modules and why they inherit direct from jboss-parent, but we certainly don't want to go there for 2.13 :)

[rsvoboda]

I vote for backport the formatting change to 2.13 to avoid troubles with backports.

I would like to see 2 commits in this PR, one for config changes + one for the changes done to .java files by impsort and auto-formatting.

For 2.13 branch we may need to backport just the config changes commit and run the build to apply the changes as 2.13...main shows 227 commits.

[gsmet]

I would like to see 2 commits in this PR, one for config changes + one for the changes done to .java files by impsort and auto-formatting.

Yeah unfortunately, that is a bad idea. Because you would then have a commit that completely fails the build which is annoying when you bissect.

So let's do one commit but @holly-cummins could you create a specific 2.13 PR once you're done with this one. By applying the rule changes and then formatting specifically for 2.13?
And maybe let's make sure you can fix the current formatting errors because apparently the build is still complaining.

[holly-cummins]

Will do on both counts - thanks @gsmet . I think the current failures are a cache issue; the impsort plugin doesn't invalidate the cache if its own settings change... so things look great locally and then fail in the build.

I'd wiped caches two levels down, but I was lazy and didn't do a find -exec to wipe them, so I missed caches three folder levels down. Another 569 deltas incoming!

[aloubyansky]

+1 for backporting to 2.13

[holly-cummins]

This is looking good to push now, except that I'll need to rebase (again). I'll rebase just before we want to merge, since otherwise it will just keep spinning off big builds.

There are about 5 poms with changes, and it's worth looking at those, since I did make a few mistakes in my original edits. The formatting changes are - hopefully - very boring.

[holly-cummins]

I've rebased, and this is looking good to merge, imo.

[quarkus-bot]

Failing Jobs - Building 8398d79

Status	Name	Step	Failures	Logs	Raw logs
✔️	Maven Tests - JDK 11
⌛	Maven Tests - JDK 11 Windows	Build	⚠️ Check →	Logs	Raw logs