maven-apache-parent icon indicating copy to clipboard operation
maven-apache-parent copied to clipboard

[MPOM-451] Remove repository elements from Apache Parent

Open lprimak opened this issue 2 years ago • 25 comments

Snapshot repositories interfere with downstream builds

lprimak avatar Dec 25 '23 23:12 lprimak

How do these interfere with downstream builds? Shouldn't they only impact SNAPSHOT builds which, since they are for non-released code, should not have any impact on downstream?

ctubbsii avatar Dec 26 '23 05:12 ctubbsii

For example, in Apache Shiro, they download SNAPSHOT versions from the repository during CI builds instead of using the local CI-build artifacts.

lprimak avatar Dec 26 '23 06:12 lprimak

This sounds like an issue with your reactor then. Maven only tries to download dependencies which are not part of the current multimodule build. Some projects from ASF rely on this inherited repo from ASF parent so just removing it is a backwards incompatible change for those.

kwin avatar Dec 26 '23 07:12 kwin

This sounds like an issue with your reactor then.

Not sure what it could possibly be. It took me 3+ hours of debugging to figure out why the builds weren't working correctly when I finally found that this was the issue. Not sure I have it in me to spend more hours figuring this out.

However, it's pretty clear that the repository section in POMs are frowned upon. I realize that this update is incompatible but IMHO this needs to be done for multiple reasons

lprimak avatar Dec 26 '23 18:12 lprimak

IMHO adding the ASF snapshot repo shouldn't do any harm for projects not leveraging it. So let us rather track the downstream issues separately.

kwin avatar Dec 26 '23 18:12 kwin

There is also a matter of pragmatism here. The repository entries really shouldn't be there in the first place. No matter what the downstream issues are, those should be removed anyway IMHO

lprimak avatar Dec 26 '23 18:12 lprimak

Please give some reasons, except for unclear downstream issues. This is very helpful for most ASF projects is my argument for keeping it in that place!

kwin avatar Dec 26 '23 18:12 kwin

Just did a quick google search "why repository in pom bad" and found this:

https://blog.sonatype.com/2009/02/why-putting-repositories-in-your-poms-is-a-bad-idea/#:~:text=This%20causes%20a%20few%20problems,one%20developer%20but%20not%20another.

lprimak avatar Dec 26 '23 18:12 lprimak

Also see this Slack discussion: https://the-asf.slack.com/archives/C7Q9JB404/p1703540867035689

lprimak avatar Dec 26 '23 18:12 lprimak

Maybe configure dependabot

# Add Maven Central explicitly to work around:
#   https://github.com/dependabot/dependabot-core/issues/8329
registries:
  maven-central:
    type: maven-repository
    url: https://repo.maven.apache.org/maven2

updates:
- package-ecosystem: maven
  directory: "/"
  schedule:
    interval: "daily"
  target-branch: "main"
  registries:
    - maven-central

olamy avatar Dec 27 '23 01:12 olamy

Will try that. Thank you!

Even if that works I still think the repository entries should be removed.

lprimak avatar Dec 27 '23 01:12 lprimak

@kwin See https://github.com/apache/shiro/pull/1245 for an example Introducing test failure doesn't actually produce a failure, due to maven resolver pulling the older snapshot from repo. When the next commit re-introduces disabling of the snapshot repository, the tests break as expected.

However, that breaks dependabot (maybe I introduce @olamy 's fix!) but still, a lot of work just due to the repository entry in parent pom. Better to remove it :) As you can see I am not the only one with similar problem. Perhaps @gnodet 's pending fix will alleviate this for 4.x, but 3.x is still broken.

lprimak avatar Dec 27 '23 05:12 lprimak

If these are removed, this is probably going to create a huge headache for all downstream users of this, who use the ASF parent POM to build ~~multi-module snapshots~~ multiple projects concurrently from snapshots (edited for clarity, since this doesn't apply to multi-module projects in the same reactor build, only interdependent projects built separately, perhaps sequentially).

ctubbsii avatar Jan 03 '24 00:01 ctubbsii

this is probably going to create a huge headache

Why? It's not difficult to put those lines into settings.xml in their own profile, on the CI system where they belong

lprimak avatar Jan 03 '24 00:01 lprimak

this is probably going to create a huge headache

Why? It's not difficult to put those lines into settings.xml in their own profile, on the CI system where they belong

It's not a difficultly issue... it's a scale problem. A lot of projects rely on this being already set up in the parent POM.

ctubbsii avatar Jan 03 '24 16:01 ctubbsii

A lot of projects rely on this being already set up

Understandable, but since these upgrades don't happen automatically and "fail fast", I don't see this as a big issue.

lprimak avatar Jan 03 '24 16:01 lprimak

A lot of projects rely on this being already set up

Understandable, but since these upgrades don't happen automatically and "fail fast", I don't see this as a big issue.

I don't see this as "fail fast"... bumping the parent version is trivial, but the requirement that every user on a project set up a repository in their local workspace or in each project and in any automated builder environment, like Jenkins, can happen later, when a non-reactor snapshot is added (typically done while testing a bugfix in a dependency prior to that dependency's release, or when co-releasing projects at the same time).

After reading all the arguments listed in favor of this, I think it boils down to:

  1. Weird behavior with dependabot that seems to only affect a few people, for which there is a workaround, and
  2. General advice against doing it because it could be slow... but this argument falls flat when the suggestion is that everybody still needs to set it up locally, and this doesn't come in at all for releases, which don't depend on snapshots.

I'm just not convinced by the arguments in favor of doing this, and worry about the impact. It's been this way for so long, without any problems whatsoever.

ctubbsii avatar Jan 04 '24 20:01 ctubbsii

this causes too much friction

I still don't see this friction. Let's look at the actual use case where this is used. The only use case I see is when the Apache project being built depends on another Apache project's SNAPSHOT build. How many instances of this use case actually exist? My bet is not many. I still see the impact of this change as very minimal.

Am I missing something glaring here?

Thanks!

lprimak avatar Jan 15 '24 21:01 lprimak

Every multi repo ASF project with inter repo dependencies needs that from time to time.

kwin avatar Jan 15 '24 21:01 kwin

Is this really a lot of impact? Also, doesn't Apache Jenkins have this built-in as well?

lprimak avatar Jan 15 '24 22:01 lprimak

create a huge headache for all downstream users of this, who use the ASF parent POM to build multi-module snapshots

I also want to reiterate that multi-module projects would not be affected at all. Only multi-repository ones that are built separately and are not using Apache Jenkins

lprimak avatar Jan 16 '24 08:01 lprimak

Local builds are affected, GitHub actions are affected (everything which does not use a centrally managed settings.xml with that repo in it), so yes from my perspective the impact is big.

kwin avatar Jan 16 '24 08:01 kwin

Local builds are affected

Not sure how. Usually local builds are either multi-module or 'installed' and do not use snapshot repository anyway.

GH actions don't use snapshot repository either.

Do you have an example project in mind that you think would be affected? How many do you think? Is this a guess or do you have some concrete idea of how many projects? and which ones?

I'd be happy to help fix any issues that would arise.

lprimak avatar Jan 16 '24 09:01 lprimak

Local builds are affected

Not sure how. Usually local builds are either multi-module or 'installed' and do not use snapshot repository anyway.

This isn't always true. For example, Apache Accumulo is configured in ci-build Jenkins to publish daily snapshots from the main accumulo repository (if a change has been made since the last snapshot). These snapshots can then be used to test Apache Fluo, which is a project that builds on top of Accumulo. Similarly, Apache Fluo can be similarly configured to publish snapshots so that Apache Rya can use them, and so on. People don't always do mvn install on everything in their dependency chain when they are doing development on a project that builds on other projects. That would require a lot of aggregate expertise as you go on downstream, and a lot of extra time and wasted effort. Snapshots published for other downstream project developers are very useful.

Similarly, if/when a snapshot build of a maven plugin is published, downstream projects can test to see if a specific bug in a plugin was fixed correctly. The same can happen with Apache commons libraries (I think I remember testing a bugfix for commons-vfs2 this way a long time ago).

So, snapshots are very useful for sharing build snapshots between projects. They can also be useful across multiple repos in a single project. For example, Accumulo also uses our snapshot builds to test our examples repository, and to run some more complicated test suites that are stored in a separate testing repo.

In short, there's a lot of use cases for these published snapshots, and developers do not limit themselves to just doing local installs for testing/development.

Yes, all of these developers could adapt and start manually configuring all their repositories. But that is friction for them, even if it's not for everybody.

GH actions don't use snapshot repository either.

Yes, absolutely they do. This is precisely how accumulo-testing repo is configured to test accumulo snapshots: See https://github.com/apache/accumulo-testing/blob/main/pom.xml#L35 and https://github.com/apache/accumulo-testing/actions

Do you have an example project in mind that you think would be affected? How many do you think? Is this a guess or do you have some concrete idea of how many projects? and which ones?

I'd be happy to help fix any issues that would arise.

It's not a complexity problem. It's a scale problem. I think most people could adapt pretty easily, without any help. It's also just a matter of convenience on a large scale (I don't know how large). Most people don't seem to be hitting the problems with dependabot that you seem to, and are content with the convenience of having it there. It's not causing a problem for everybody, and not everybody considers it worth changing, because it's more convenient the way it is.

Personally, on a scale of [-1.0, +1.0], I'm probably a -0.25 opposed to this change. I could adapt if it were removed, but I think there's enough people with a slightly negative view of this change, that their total inconvenience is probably more substantial in aggregate than the few who are strongly in favor of this change.

ctubbsii avatar Jan 23 '24 00:01 ctubbsii

@lprimak fwiw, I think interactions between the local build and the local repository and remote repositories is not new. Maven 4 tries to solve the problem with the introduction of a project local repository in ${session.rootDirectory}/target/project-local-repo which stores the artifacts created by the project, so that artifacts produced within the reactor should never be picked up from the local or remote repositories.

gnodet avatar Sep 04 '24 09:09 gnodet

Since Maven 4 is about to be released and break everything anyway, it's time to merge this as well

lprimak avatar Dec 17 '24 16:12 lprimak

What about moving the snapshot repository in a profile activated using a known property ? Projects depending on the repo could easily activate the profile by defining the needed property, while those that are not interested in the snapshot repo simply not define the property.

gnodet avatar Dec 17 '24 22:12 gnodet

What about moving the snapshot repository in a profile activated using a known property ? Projects depending on the repo could easily activate the profile by defining the needed property, while those that are not interested in the snapshot repo simply not define the property.

That’s a fantastic idea. I’ll make it happen.

lprimak avatar Dec 17 '24 23:12 lprimak

Done

lprimak avatar Dec 18 '24 01:12 lprimak

I updated the description to better explain issues with the status quo

lprimak avatar Dec 18 '24 21:12 lprimak