Skip to content

If getting local repo from settings.xml, do it right #360

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

Closed
wants to merge 1 commit into from

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Feb 2, 2024

There are people who intentionally use non-default local repository locations, plus, they may have some expressions in the settings.xml...

So, if you do utilize code to get local repo from settings.xml, do it right. For me this whole stuff was not buildable due my (otherwise valid) settings.xml and non-default location of my local repository (that is on purpose not in default location).

@cstamas cstamas self-assigned this Feb 2, 2024
@gnodet
Copy link
Member

gnodet commented Feb 2, 2024

I would think the correct way to build settings is to use SettingsBuilder.

Though this is only used during tests, so that should be fine...

@cstamas
Copy link
Member Author

cstamas commented Feb 2, 2024

Ideally, I'd drop all the deprecated stuff from maven-artifact as well (ArtifactRepo, layout...) as ultimately the goal here is "just" to get a file from local repo... To make it "proper" using SettingsBuilder and maybe SimpleLocalRepositoryManager from resolver, that would need all of maven and resolver (almost) just to perform this single thing...

Maybe the UT is wrong, and all we need is just to have this commons artifact under src/test/resources instead?

@kwin
Copy link
Contributor

kwin commented Feb 2, 2024

There is an easier solution in #357, please have a look.

@cstamas
Copy link
Member Author

cstamas commented Feb 2, 2024

Did you try with settings.xml having ${env.XXX} inside? 😉

@slawekjaranowski
Copy link
Member

IMHO tests are wrong, local repo is used to discover path for artifacts to build classpath ...
We need to check if we can simply give any path for assertions

@kwin
Copy link
Contributor

kwin commented Feb 2, 2024

Did you try with settings.xml having ${env.XXX} inside? 😉

Yes, because the SettingsBuilder does the interpolation (https://github.com/apache/maven/blob/42559937d8c4ed8a361422a8db3dfa6ef81fef21/maven-settings-builder/src/main/java/org/apache/maven/settings/building/DefaultSettingsBuilder.java#L235)

@slawekjaranowski
Copy link
Member

suppressed by ##366 - now pass path to local repo from executing Maven

@cstamas cstamas closed this Feb 17, 2024
@cstamas cstamas deleted the fix-local-repo-in-tests branch February 17, 2024 15:38
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.

4 participants