Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
    Jan Eglinger
    @imagejan
    so we need a “high-level” context creation test as well
    Mark Hiner
    @hinerm
    sorry, between meetings
    the problem is in ij1-patcher I believe
    in the handleMacAdapter
    I fixed ij1-builds but not ij1-patcher for the package change
    that should be all
    Mark Hiner
    @hinerm

    Another problem might be that this exception is only thrown when run on Mac, right? So the usual CI build won’t catch it…

    This is correct

    so we need a “high-level” context creation test as well

    This should implicitly happen in the melting pot. But the problem we ran into here isn't specifically about context creation. Any test in imagej-legacy or downstream I would expect to hit this exception.. but as you noted, nothing is testing on mac

    to which repository should I add the test?

    @tpietzsch I'm sorry, I just wanted you to add the test to the bdv-fiji branch to complete the MCVE. But since we identified the problem and have now fixed it (imagej/ij1-patcher@0f3c057) I don't think we need to add a test anywhere

    Mark Hiner
    @hinerm
    @tpietzsch would you mind checking out the latest https://github.com/imagej/ij1-patcher and running mvn clean test to see if it works on mac?
    and maybe also repeating for imagej/ij1-patcher@d45f936 ? which I assume will fail because it lacks the mac adapter
    Tobias Pietzsch
    @tpietzsch
    yes to both
    master goes through
    imagej/ij1-patcher@d45f936 fails
    Mark Hiner
    @hinerm
    :+1: thank you!
    Tobias Pietzsch
    @tpietzsch
    The weird thing is that Java 8 complains about the MacAdapter thing
    Java 11 does not
    https://gist.github.com/tpietzsch/dc728e693f24a5a8d1453e39ce341e3b
    has the output from both java versions (on imagej/ij1-patcher@d45f936)
    For Java 11, some tests go through, e.g. 
    [INFO] Running net.imagej.patcher.LegacyHooksTest
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.713 s - in net.imagej.patcher.LegacyHooksTest
    For Java 8, the same test fails 
    [INFO] Running net.imagej.patcher.LegacyHooksTest
java.lang.RuntimeException: Found incompatible ij.IJ class
    at net.imagej.patcher.LegacyEnvironment.initialize(LegacyEnvironment.java:112)
    at net.imagej.patcher.LegacyEnvironment.applyPatches(LegacyEnvironment.java:494)
...
Caused by: java.lang.IllegalArgumentException: No such class: MacAdapter
    at net.imagej.patcher.CodeHacker.getClass(CodeHacker.java:878)
    at net.imagej.patcher.CodeHacker.getMethod(CodeHacker.java:904)
    at net.imagej.patcher.CodeHacker.getBehavior(CodeHacker.java:894)
    at net.imagej.patcher.CodeHacker.insertAtTopOfMethod(CodeHacker.java:157)
    ... 35 more
Caused by: javassist.NotFoundException: MacAdapter
    at javassist.ClassPool.get(ClassPool.java:430)
    at net.imagej.patcher.CodeHacker.getClass(CodeHacker.java:873)
    ... 38 more
[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.287 s <<< FAILURE! - in net.imagej.patcher.LegacyHooksTest
    Super weird
    Mark Hiner
    @hinerm
    @tpietzsch something about MacAdapter is java-8 specific IIRC
    but I can't find the code that suggests that
    anyway I'm releasing a new ij1-patcher and this will all be happier in the next pom-scijava
    currently waiting on trakem2/TrakEM2#28 and PreibischLab/multiview-reconstruction#32
    Jan Eglinger
    @imagejan
    :+1: thanks for all of this, @hinerm !
    Mark Hiner
    @hinerm
    hey you fixed it first @imagejan! Sorry I missed your PR
    @tpietzsch I can bump BDV-core to 10.2.0.. are there any other BDV versions you want updated?
    Jan Eglinger
    @imagejan

    no need to apologise! If I remember correctly, Curtis tried to keep ij1-patcher compatible to as many versions of ij.jar as possible, i.e. also for older versions. I think that’s not really tenable with the latest package changes of MacAdapter and other changes in IJ1. But I think the current ij1-patcher at least doesn’t throw errors when used in non-headless mode with older versions of ij.jar.

    Also, we should try to catch up with the newer versions up to 1.53g and soon h :-/

    Mark Hiner
    @hinerm

    Also, we should try to catch up with the newer versions up to 1.53g and soon h :-/

    :skull: :skull: :skull: :ghost:

    Jan Eglinger
    @imagejan
    :laughing:
    Tobias Pietzsch
    @tpietzsch
    @hinerm yes bigdataviewer-core-10.2.0
    bigdataviewer-vistools-1.0.0-beta-27
    Mark Hiner
    @hinerm
    @tpietzsch :+1: I'll run a melting pot with those right now
    Tobias Pietzsch
    @tpietzsch
    and I'm releasing bigdataviewer_fiji-6.2.1 now
    that would be also good, if you can still get it in
    (but this is not that important, nothing should depend on bigdataviewer_fiji)
    so bigdataviewer_fiji-6.2.0 would be fine too
    thanks for all of this, @hinerm !
    Tobias Pietzsch
    @tpietzsch
    yes, thanks a lot!!!
    Mark Hiner
    @hinerm
    I wonder if there's a way to detect pinned versions...
    melting-pot is nice for testing, but I feel like it would be helpful if we had PRs to scijava-common whenever people had to pin versions
    Jan Eglinger
    @imagejan
    Hm, sometimes you need to pin to a newer version of a dependency, but you wouldn’t like to have that in pom-scijava immediately, because it might break other dependencies, no? On the other hand, having a bot that creates a new PR to pom-scijava whenever you release a component that has a pinned (newer overridden) dependency would be handy…
    But I think already having automated mega-melt being run for every PR to pom-scijava will be a good thing. I see no problem with developers filing PRs “manually” for concerted updates of groups of components (e.g. the n5 ecosystem, ot the BDV ecosystem).
    Mark Hiner
    @hinerm

    but you wouldn’t like to have that in pom-scijava immediately, because it might break other dependencies, no?

    In general, pinning to dependencies is a very bad thing that we should strive to avoid, because we only get to put one version of everything on core Fiji - being whichever version is in pom-scijava

    Making a change in pom-scijava that breaks other things is not a bad thing. As long as we're testing with melting-pot before uploading, we aren't going to break Fiji

    But I think already having automated mega-melt being run for every PR to pom-scijava will be a good thing.

    This would be fine but I don't think we need to reject a change because it breaks something else. Others may disagree, but personally I just care that everything is coherent at release time

    Mark Hiner
    @hinerm

    I see no problem with developers filing PRs “manually” for concerted updates of groups of components (e.g. the n5 ecosystem, ot the BDV ecosystem).

    totally. I'd be happy just to have the melting pot report if it finds any "Overriding managed version" warnings.