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

[MPOM-451] Remove repository elements from Apache Parent #183

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lprimak
Copy link
Contributor

@lprimak lprimak commented Dec 25, 2023

Snapshot repositories interfere with downstream builds

@ctubbsii
Copy link
Member

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?

@lprimak
Copy link
Contributor Author

lprimak commented Dec 26, 2023

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

@kwin
Copy link
Member

kwin commented Dec 26, 2023

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.

@lprimak
Copy link
Contributor Author

lprimak commented Dec 26, 2023

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

@kwin
Copy link
Member

kwin commented Dec 26, 2023

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.

@lprimak
Copy link
Contributor Author

lprimak commented Dec 26, 2023

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

@kwin
Copy link
Member

kwin commented Dec 26, 2023

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!

@lprimak
Copy link
Contributor Author

lprimak commented Dec 26, 2023

@lprimak
Copy link
Contributor Author

lprimak commented Dec 26, 2023

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

@olamy
Copy link
Member

olamy commented Dec 27, 2023

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

@lprimak
Copy link
Contributor Author

lprimak commented Dec 27, 2023

Will try that. Thank you!

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

@lprimak
Copy link
Contributor Author

lprimak commented Dec 27, 2023

@kwin See apache/shiro#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.

@ctubbsii
Copy link
Member

ctubbsii commented Jan 3, 2024

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).

@lprimak
Copy link
Contributor Author

lprimak commented Jan 3, 2024

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

@ctubbsii
Copy link
Member

ctubbsii commented Jan 3, 2024

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.

@lprimak
Copy link
Contributor Author

lprimak commented Jan 3, 2024

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.

@ctubbsii
Copy link
Member

ctubbsii commented Jan 4, 2024

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.

Copy link
Member

@kwin kwin left a comment

Choose a reason for hiding this comment

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

As commented before I think this causes too much friction for ASF parent consumer who want to upgrade and leverage those repositories. Let us rather rely on dependabot coming up with a proper fix.

@lprimak
Copy link
Contributor Author

lprimak commented Jan 15, 2024

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!

@kwin
Copy link
Member

kwin commented Jan 15, 2024

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

@lprimak
Copy link
Contributor Author

lprimak commented Jan 15, 2024

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

@lprimak
Copy link
Contributor Author

lprimak commented Jan 16, 2024

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

@kwin
Copy link
Member

kwin commented Jan 16, 2024

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.

@lprimak
Copy link
Contributor Author

lprimak commented Jan 16, 2024

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.

@ctubbsii
Copy link
Member

ctubbsii commented Jan 23, 2024

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.

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

Successfully merging this pull request may close these issues.

None yet

4 participants