#49 use jcabi-aether for dependency resolving

[andreas-mausch]

Sample output:

$ kscript /home/neonew/Schreibtisch/kscript/test/resources/depends_on_with_type.kts
[kscript] Resolving dependencies...
[kscript]     Resolving org.javamoney:moneta:1.3@pom...Done
[kscript]     Resolving com.github.holgerbrandl:kscript-annotations:1.2...Done
[kscript] Dependencies resolved
getBigDecimal(1L): 1

• Updated shadow plugin to 2.0.4 (otherwise the build is extremely slow)
• Resulting kscript.jar went from 122kb -> 18.5mb :(
• I've excluded some (unused?) dependencies to strip it down to 5.1mb
• No requirement for mvn anymore

Some excludes were even needed, otherwise we would see:
Sep 07, 2018 4:03:24 AM org.hibernate.validator.internal.util.Version
INFO: HV000001: Hibernate Validator 5.1.2.Final
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.

Feedback is welcomed.

[holgerbrandl]

Great PR, thanks for this lovely piece of work.

Will it reuse the local .m2 cache as commented on by @gildor in #49?

Some (@stephenbyrne-mfj) commented that aether is no longer under development. So hopefully we're not betting on the wrong horse here.

I am mostly worried about backward compatibility but it's hard/tedious to test it. So I'll just test the PR locally for a few days, before merging it into master.

[andreas-mausch]

Will it reuse the local .m2 cache

Well, that's a bit of a problem. Currently I do:

File(System.getProperty("user.home") + "/.m2/repository")

So M2_HOME can be different than ~/.m2 and I currently ignore any settings.xml which overwrites the localRepo location.
Won't be a problem for most users though.

aether is no longer under development

True. Gildor mentioned "Also, even Gradle uses Aether for maven (at least for publication, as I see, but cannot be sure)."
But absolutely, this is a risk.
I've addtionally used jcabi-aether as a wrapper, which has a couple of commits other the last year, but not sure how well it is maintained overall.

I am mostly worried about backward compatibility but it's hard/tedious to test it.

I have the same doubts, and I won't be disappointed if you decide to stick with the maven solution.
This PR doesn't win much (maybe a bit ramp-up time because we can skip pom parsing) and is a bit risky.

I just wanted to share it and hey, maybe it's working well.
If you decide you want to merge it, I will definitely check it doesn't break the dependencies.txt.

[dimiii]

@andreas-mausch Could you add some tests, please?

[andreas-mausch]

@dimiii Do you have a special test in mind?

I see this more as a refactoring. All scripts in the test folder use the new method, so as long as they are green, it should be fine.

[holgerbrandl]

I agree with @andreas-mausch , as long as the tests are green the PR is ready for merging.

Sorry for the delayed process. But since kscript maybe merged into the offical kotlin-core soon, we may want/need to use the official scripting aether integration instead.

[abelsromero]

I tested on mingw (gtibash) and resolution works, execution still fails though, but it is a great step forward.

But the Kotlin version needs to be updated to 1.2.70 (same as current master) to avoid e: Kotlin home does not exist or is not a directory: .

[holgerbrandl]

This was to be expected because of the same issue as described in #154: Before calling kotlinc we check its presence by calling bash, which fails under windows at the moment.

[abelsromero]

On thing at a time, what is needed to merge this?

[holgerbrandl]

Not much, it's a great PR. There's two arguments

1. It's not adding anything new from a user-perspective, and why change a running system?
2. With @ligee's effort to port kscript to the new Kotlin scripting API (since v1.3), I guess the PR would become obsolete, because the scripting API has its own aether integration for dependency resolution.

Clearly kscript failing on windows somehow invalidates (1) but the PR would imho not fix this entirely because execution would still fail on windows.

[abelsromero]

2\. With @ligee's effort to port `kscript` to the new Kotlin scripting API (since v1.3), I guess the PR would become obsolete

Would not the whole project become obsolete?

[holgerbrandl]

The idea is rather to make it part of the core distribution of Kotlin :-).

So technically this repo would become obsolete, but conceptually it would imho be a great grand finale for the idea of providing an easy-to-use, very flexible, and almost zero-overhead solution to write self-contained mini-applications with Kotlin. (quoting myself from the readme)

it's an ongoing process so let's see if we manage. Until then I'll keep working on it.

[abelsromero]

Glad to hear that! I'd like to give it a try to fix the issues to make it run with Windows.
And it will be much easier if this PR gets merged.

[abelsromero]

I have fixed most of Windows issues (https://github.com/abelsromero/kscript/tree/add_windows_support) but I am finding problems with the current maven resolution due to issues parsing the output of the mvn command from cygwin (line ending + pathing stuff). Honestly, I don't see the point in fixing those having this PR which makes the resolution much more stable and clean.

So, apologies if I sound rude, I just want to be 100% clear here, so I ask can this PR be merged?. As soon as this is merged I can rebase and test it with my fixes. Note I am also testing on Mac and Linux, and checking with local_test_suite.sh.

Answering the arguments given before:

1. It's not adding anything new from a user-perspective, and why change a running system?
2. With @ligee's effort to port kscript to the new Kotlin scripting API (since v1.3), I guess the PR would become obsolete, because the scripting API has its own aether integration for dependency resolution.

1. This is clearly a big improvement and removes depending on maven.
2. So long there's no clear date on Kotlin 1.3 release, kscript is the only way to provide kotlin scripting. Not to mention this can be a tool for kotlin <1.3, I don't think this is going to become obsolete.

[abelsromero]

Apologies again, I admit I was a bit heated yesterday and overreacted.
However, I maintain my request so we can provide Windows compat.

[holgerbrandl]

I agree waiting for v1.3 or some future conversion/migration is not really helpful. So let's do it then. I've cleaned most open tickets this morning and pushed out a new release, so that we can focus on this PR now.

We need to

1. Fix the conflicts in the PR with master
2. Add some tests for dependency notation does not support maven version range syntax containing ) and , #166 and feature: Add credentials to @file:MavenRepository #143. Both relate to reoccurring reported use-cases so we should make sure that aether would support them in some sensible way. They are not supported yet, but could be adressed by moving the dependency resolution from mvn to gradle, so I'd love to ensure that aether allows for the same functionality.
3. Merge it. :-)

I think your windows-compat improvements (thanks!) should be done separately (like in a different PR/branch). Not sure if we should merge before or after this one. What do you think? We also need to think about testability: Is there any way we can test windows compat on Travis or some other CI platform that integrates with github?

[abelsromero]

Thanks for you comprehension.

I think your windows-compat improvements (thanks!) should be done separately (like in a different PR/branch). Not sure if we should merge before or after this one. What do you think?

I agree it should be a separate PR, but after this one, I found issues with running mvn command that honestly I had not clue. For instance, some poms resolved correctly, others not, but only form shell, running from a junit they resolved correctly.
But this raises the question @andreas-mausch, can you take care of the extra tests and conflicts?

Is there any way we can test windows compat on Travis or some other CI platform that integrates with github?

Travis has just recently added windows, or else we can integrate AppVeyor. I have used AppVeyor in many project before, shouldn't be hard to begin with. I can add that to the windows PR, but you'll have to enable it for your repo.

[andreas-mausch]

@abelsromero Hi, luckily I'm on vacation for the next weeks, so I should find some time to take care of the conflicts.
For the additional features: I need to look into them, but from my first impression they seem to be simple.

From my memory: There was also an issue to support a version format like the "1.2+" notation, like gradle supports it. Imo this will require some more effort and I wouldn't implement it (right now). I also really dislike non-fixed versions, so it's also a matter of my personal choice here. ;) Just want to mention it because of "moving the dependency resolution from mvn to gradle".

[andreas-mausch]

Conflicts in the PR with master: Resolved.

[holgerbrandl]

Thanks @andreas-mausch for this great PR and sorry again for the endless delay on my end.

[abelsromero]

sorry again for the endless delay on my end.

I missed that too 🤦‍♂️

[andreas-mausch]

Np, glad you like it. Thanks for merging.