search

benedekfazekas / mranderson

Dependency inlining and shadowing
Eclipse Public License 1.0
151 stars 14 forks source link

Simplify parallelism #59

Closed vemv closed 3 years ago

vemv commented 3 years ago
codecov-commenter commented 3 years ago

Codecov Report

Merging #59 (dc0f914) into master (57f01e2) will decrease coverage by 0.19%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #59      +/-   ##
==========================================
- Coverage   64.46%   64.26%   -0.20%     
==========================================
  Files           7        7              
  Lines         695      694       -1     
  Branches       45       46       +1     
==========================================
- Hits          448      446       -2     
  Misses        202      202              
- Partials       45       46       +1
Impacted Files Coverage Δ
src/mranderson/util.clj 54.83% <ø> (ø)
src/mranderson/move.clj 91.75% <100.00%> (-0.56%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 57f01e2...dc0f914. Read the comment docs.

benedekfazekas commented 3 years ago

did a bit of testing on cider-nrepl and I don't think this change really solves #57 -- it may solve #56 but i have not tested that. when i try to build a mranderson jar based on this branch with java16 and try to inline cider-nrepl with it, it still takes at least 30mins on my machine which should not be the case. I also tried with delays and played around with parallel too a bit: the same. guessing really figuring out what is the problem described in #57 needs a bit more digging.

that said i am not necessarily against merging this. will do an other round of testing on it

vemv commented 3 years ago

Cheers. It seems worth it to add cider-nrepl to the CI suite as an e2e test and see if we can repro the slowness in Circle.

Will do, normally these are easy to set up

benedekfazekas commented 3 years ago

good idea, thanks

vemv commented 3 years ago

Drafted the cider-nrepl test and it brought one interesting failure so far...

java.lang.ClassCastException: org.pantsbuild.jarjar.Rule cannot be cast to com.tonicsystems.jarjar.PatternElement

https://app.circleci.com/pipelines/github/reducecombine/mranderson/11/workflows/b8539722-058d-4ef4-b4f0-b7297391b1d6/jobs/25

Does it ring a bell?

benedekfazekas commented 3 years ago

this coming from the integration with the jarjar tool that rewrites/prefixes packages names in java class files. the integration was changed (the library used upgraded) lately in this commit 57df48da748618e69b136cedb0bfec25853f986f

vemv commented 3 years ago

Mmm, let's see what we can do. That one commit seems a bit delicate since it didn't have a CI matrix backing it's well possible that it broke something?

Maybe we can have both libs/impls around. I'd assume that the old impl is a 'known-good' one which could be dynamically selected depending on the JDK detected at runtime.

vemv commented 3 years ago

I'm finding in a refactor-nrepl branch that only JDK17 builds are passing, which confirms that indeed there's a problem with the mrandersoning / its relationship to JDKs:

image

benedekfazekas commented 3 years ago

these may be two separate problems. one of them inlining being really slow on JVMs versions > 11 and java class prefixing (that is the jarjar thing) failing on older JVMs (< 17). my hunch is that these problems are not related because they are two separate steps in the inlining process. it is strange that i have not realised the failure with older JVMs when testing the above mentioned commit/pr

vemv commented 3 years ago

I haven't such detected issues with mranderson for a few months, however dependency trees can always get more complicated which surfaces harder edge cases.

In my case, I'm bumping rewrite-clj which has changed quite a lot in since its previous version. So that can explain things.

vemv commented 3 years ago

Agreed that they're most likely separate problems!

This PR claims to solve no particular problem at all btw for the time being I'm simply seeking to leave things a little clearer and more tested.

benedekfazekas commented 3 years ago

kinda thinking out loud really, your looking at this is really appreciated 🙇

benedekfazekas commented 3 years ago

my guess is that you are not really testing with mranderson just built in the pipeline. suppose conj only puts the new plugin version after the one already there so the original is used?! the code line the trace in the build error refers to tonicssystems but tonicsystems is not there in the mentioned file on master

vemv commented 3 years ago

I was assuming that in presence of two identically named plugins, Lein picks one, namely the most recent one.

Worth verifying though 👀

vemv commented 3 years ago

So I tentatively reverted https://github.com/benedekfazekas/mranderson/pull/51 and succeeded! Build is green now.

However that pull intented to solve https://github.com/benedekfazekas/mranderson/issues/48 . Maybe it solved it but didn't add specific test coverage?

I'll try to proceed with that. Probably the final incantation will have to choose impls dynamically as mentioned.

benedekfazekas commented 3 years ago

so you sure your conj into the plugins worked as you expected?

vemv commented 3 years ago

so you sure your conj into the plugins worked as you expected?

Just in case, I replaced conj with a patch. The build is green, however I had to make this change in the integration test:

-lein with-profile -user,-dev,+plugin.mranderson/config install
+lein with-profile -user,-dev install

That seems suspicious, so I wouldn't be confident yet about this PR.

Anyway, it'd be useful for me to understand - why is the plugin.mranderson/config profile needed for alein install? i.e. why does mranderson have to be mrandersonized itself?

That wouldn't seem particularly necessary for a Lein plugin.

vemv commented 3 years ago

Anyway, it'd be useful for me to understand - why is the plugin.mranderson/config profile needed for alein install?

Ok, mistery solved

There was this discrepancy between the project version and the :plugins version here:

image

The commit named Bootstrap installation properly solves this. It performs lein install in two steps (without and then with plugin) so that one doesn't depend on a prior mranderson version, which seems error-prone.

So now the PR is green, matrix'd and without dirty conj.

I still should try to repro the issue reported in #48.

vemv commented 3 years ago

This PR was a bit messier than planned so I extracted the essentials to https://github.com/benedekfazekas/mranderson/pull/60