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

buildMaven: Check for authenticated attribute #44091

Merged
merged 1 commit into from Jul 25, 2018

Conversation

asymmetric
Copy link
Contributor

@asymmetric asymmetric commented Jul 25, 2018

Motivation for this change

The authenticated attribute is not always present in the
project-info.json produced by maven2nix0

We therefore check for its presence, and default it to false.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

The `authenticated` attribute is not always present in the
`project-info.json` produced by maven2nix[0]

We therefore check for its presence, and default it to false.

[0]: NixOS/mvn2nix-maven-plugin#5 (comment)
@ttuegel
Copy link
Member

ttuegel commented Jul 25, 2018

There is actually a deeper problem here, which is that if authenticated is not set, then mvn2nix doesn't know what remote repository the dependency belongs to.

@asymmetric asymmetric deleted the asymmetric/maven-authn branch July 26, 2018 00:17
@jerith666
Copy link
Contributor

Not sure I follow ... authenticated is written out by mvn2nix, right? Do you mean that buildMaven doesn't know what repo the dep belongs to?

@ttuegel
Copy link
Member

ttuegel commented Jul 26, 2018

Not sure I follow ... authenticated is written out by mvn2nix, right?

mvn2nix writes authenticated for each dependency from a remote repository, at the same time as it writes the other information for that remote repository (such as URL). For local dependencies, mvn2nix does not write authenticated or any remote repository information because that information isn't available. Unfortunately, maven sometimes incorrectly reports remote dependencies as local dependencies (*). If authenticated is missing, that is usually the reason. Nix cannot fetch the dependency because mvn2nix thinks it is locally available, and so the builder will fail due to the missing dependency.

TL;DR: this pull request fixes evaluation, but not the build.

(*) For our purposes, the main reason maven reports remote dependencies as local dependencies is if they are cached locally. I consider this a serious flaw; caching should be transparent!

@jerith666
Copy link
Contributor

Ah, okay, gotcha -- that makes sense, thanks for figuring it out. (And I agree it's a serious flaw in Maven!)

So in some sense it's yet another reason to use the -Dmaven.repo.local=$(mktemp -d -t maven) work-around proposed at NixOS/mvn2nix-maven-plugin#5, I guess?

FWIW, I'm still not entirely convinced that a maven plugin is the right way to figure out the complete set of maven dependencies for a project, at least not without some upstream work on maven itself. See discussion at #19741.

Maybe it's worth a post to the maven dev team's mailing list to try to figure some of this out with their help? Even something general like "hey, what would it take to get maven to produce a lock file like npm can?" ...

@asymmetric
Copy link
Contributor Author

So in some sense it's yet another reason to use the -Dmaven.repo.local=$(mktemp -d -t maven) work-around proposed at NixOS/mvn2nix-maven-plugin#5, I guess?

Does it work for you though? I've just tried it on the K repo and it didn't.

@jerith666
Copy link
Contributor

I haven't tried it, no. :-/

@ttuegel
Copy link
Member

ttuegel commented Jul 26, 2018

Does it work for you though? I've just tried it on the K repo and it didn't.

It does not, because of NixOS/mvn2nix-maven-plugin#15.

I fixed that, and it still does not work because maven will always resolve the mvn2nix dependencies before it runs the tool. There will always ALWAYS be cached dependencies that mvn2nix will miss if your package has transitive dependencies in common with mvn2nix.

I think the only way forward is to fetch the dependencies through maven itself. I am working on that now. I am still having problems because the K pom.xml file doesn't actually list all its dependencies, but that is a much simpler problem!

@asymmetric
Copy link
Contributor Author

@ttuegel are you using dependency:go-offline? Or what other approach are you exploring?

There are 2 existing attempts using dependency:go-offline:

@jerith666
Copy link
Contributor

Sorry for the confusion -- my script in #19741 is meant for after running a regular build, not dependency:go-offline. My experience was that go-offline didn't always download everything you really needed, only an actual build would do that. I've updated over there to reflect that.

@ttuegel
Copy link
Member

ttuegel commented Jul 28, 2018

My experience was that go-offline didn't always download everything you really needed, only an actual build would do that.

Yes, I found that. However, a newer version of maven-dependency-plugin seems to do a good enough job with resolve-plugins and go-offline that I can fix 95% of the problem by adding some dependencies that (admittedly) weren't specified in the pom.xml file anyway. I still can't get everything though.

@asymmetric
Copy link
Contributor Author

@ttuegel Is your work-in-progress available somewhere?

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

Successfully merging this pull request may close these issues.

None yet

4 participants