This service will be undergoing maintenance at 00:00 UTC, 2017-10-23 It is expected to last about 30 minutes
Bug 1289726 - Review Request: apache-commons-weaver - Apache Commons Weaver
Review Request: apache-commons-weaver - Apache Commons Weaver
Status: NEW
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-JAVASIG 1306675
  Show dependency treegraph
 
Reported: 2015-12-08 15:16 EST by gil cattaneo
Modified: 2016-10-28 02:23 EDT (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description gil cattaneo 2015-12-08 15:16:04 EST
Spec URL: https://gil.fedorapeople.org/apache-commons-weaver.spec
SRPM URL: https://gil.fedorapeople.org/apache-commons-weaver-1.1-1.fc23.src.rpm
Description:
Apache Commons Weaver provides an easy way to enhance compiled Java
classes by generating ("weaving") bytecode into those classes.

Fedora Account System Username: gil
Comment 1 Upstream Release Monitoring 2015-12-08 15:22:00 EST
gil's scratch build of apache-commons-weaver-1.1-1.fc23.src.rpm for rawhide completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12114229
Comment 2 Ville Skyttä 2016-01-04 03:16:30 EST
All *.txt are installed as executable.

All summaries are quite weak, they pretty much just repeat the package's name.

Most subpackage descriptions have the same issue as the summaries.

Are the test failures somehow expected and safe to just ignore? Upstream issue tracker or discussion reference?

Javadocs crosslink with online Java SE 6 javadocs. Would be better to crosslink with local java-javadoc. Ditto crosslinking with dependencies' javadocs, at least ant-javadoc and objectweb-asm-javadoc seem to be applicable ones, and there are probably more.
Comment 3 gil cattaneo 2016-01-05 05:09:12 EST
(In reply to Ville Skyttä from comment #2)
> All *.txt are installed as executable.
Fixed
> All summaries are quite weak, they pretty much just repeat the package's
> name.
> 
> Most subpackage descriptions have the same issue as the summaries.
Tried to fix

> Are the test failures somehow expected and safe to just ignore? Upstream
> issue tracker or discussion reference?
This commons-weaver release use xbean-finder 3.18. In Fedora is available xbean-finder >= 4.3. No discussion about upgrade xbean

> Javadocs crosslink with online Java SE 6 javadocs. Would be better to
> crosslink with local java-javadoc. Ditto crosslinking with dependencies'
> javadocs, at least ant-javadoc and objectweb-asm-javadoc seem to be
> applicable ones, and there are probably more.
Sorry, where you see the for "docs" task crosslink references in commons-weaver project?


Spec URL: https://gil.fedorapeople.org/apache-commons-weaver.spec
SRPM URL: https://gil.fedorapeople.org/apache-commons-weaver-1.1-2.fc23.src.rpm

- fix *.txt permissions
Comment 4 Ville Skyttä 2016-01-05 05:46:28 EST
Summaries have not been changed, the issue persists (non-blocker).

Descriptions could use a bit longer lines, see other packages (non-blocker).

Test stuff is a blocker. First, now there is some kind of an explanation for the FinderTest failure, but there's no explanation why it is fine to just disable the test; the failure seems to be directly related to the xbean-finder dependency changes made in the package. Either the code needs to be fixed so that the test passes, or the test fixed if it's that and not the code that is broken after the change. Can't simply change the dep and ignore errors resulting from doing so.

Second, on a quick look, CleanProcessorTest and WeaveProcessorTest failures do not seem to have anything to do with the xbean stuff, so there's still no explanation for why it would be ok to ignore the failures.

Regarding crosslinking, I'm just looking at the contents of the -javadoc package; there are various links in them to the online Java SE 6 javadoc. For local crosslinking, adding build deps on related *-javadoc packages and then pointing the javadoc plugin to appropriate local file:///usr/share/javadoc/* dirs would fix it, I imagine (at least it was something like that back when I last had anything to do with Java packaging). https://maven.apache.org/plugins/maven-javadoc-plugin/examples/links-configuration.html (non-blocker as I don't think there is a requirement to do this, but I do recommend fixing it as it should be quite easy)

The following subpackages seem to be missing license files: example, normalizer, normalizer-example, privilizer-api
Comment 5 Upstream Release Monitoring 2016-01-05 07:17:55 EST
gil's scratch build of apache-commons-weaver-1.2-0.1.RC1.fc23.src.rpm for rawhide failed http://koji.fedoraproject.org/koji/taskinfo?taskID=12418510
Comment 6 Upstream Release Monitoring 2016-01-05 07:32:04 EST
gil's scratch build of apache-commons-weaver-1.2-0.1.RC1.fc23.src.rpm for rawhide failed http://koji.fedoraproject.org/koji/taskinfo?taskID=12418655
Comment 7 Upstream Release Monitoring 2016-01-05 08:23:51 EST
gil's scratch build of apache-commons-weaver-1.2-0.1.RC1.fc23.src.rpm for rawhide completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12419124
Comment 9 gil cattaneo 2016-01-06 08:05:12 EST
(In reply to Ville Skyttä from comment #4)
> Test stuff is a blocker. First, now there is some kind of an explanation for
> the FinderTest failure, but there's no explanation why it is fine to just
> disable the test; the failure seems to be directly related to the
> xbean-finder dependency changes made in the package. Either the code needs
> to be fixed so that the test passes, or the test fixed if it's that and not
> the code that is broken after the change. Can't simply change the dep and
> ignore errors resulting from doing so.
> 
> Second, on a quick look, CleanProcessorTest and WeaveProcessorTest failures
> do not seem to have anything to do with the xbean stuff, so there's still no
> explanation for why it would be ok to ignore the failures.

Seem relate to JDK platform vendor. I'm waiting for some news related https://issues.apache.org/jira/browse/WEAVER-10. This kind of issues usually happen and should not considering as blocker, IMO.

> Regarding crosslinking, I'm just looking at the contents of the -javadoc
> package; there are various links in them to the online Java SE 6 javadoc.
> For local crosslinking, adding build deps on related *-javadoc packages and
> then pointing the javadoc plugin to appropriate local
> file:///usr/share/javadoc/* dirs would fix it, I imagine (at least it was
> something like that back when I last had anything to do with Java
> packaging).
> https://maven.apache.org/plugins/maven-javadoc-plugin/examples/links-
> configuration.html (non-blocker as I don't think there is a requirement to
> do this, but I do recommend fixing it as it should be quite easy)
Done
> The following subpackages seem to be missing license files: example,
> normalizer, normalizer-example, privilizer-api
Done
Comment 10 Ville Skyttä 2016-01-19 03:24:46 EST
I'm not at all convinced that it's related to the JDK platform vendor. Our OpenJDK also reports itself as "vendor: Oracle Corporation" so it might be pretty much the same as the one upstream is using.

You did not tell upstream that you've changed xbean-finder-shaded to xbean-finder and org.apache.xbean.asm5 to org.objectweb.asm. That is crucial information. I hacked your SRPM so that it does not do those changes and instead downloads xbean-finder-shaded from the network and uses it during the build, and all tests pass. So the failures are related to that change.

I don't agree with you that stuff like this is not a blocker. It is quite clear that the package does not work properly at the moment. Because the failures are in the processor component which is quite central to this one and it would not make sense to try shipping the package without it, I don't think this package should be shipped at all before the issues are resolved.
Comment 12 Ville Skyttä 2016-02-09 01:51:01 EST
I see the upstream issue report has not yet been amended with information that the failures are caused by swap of the xbean stuff. Do you plan to let them know and ask for help, or what's the plan with getting these things fixed?
Comment 13 gil cattaneo 2016-02-09 09:01:30 EST
(In reply to Ville Skyttä from comment #12)
> I see the upstream issue report has not yet been amended with information
> that the failures are caused by swap of the xbean stuff. Do you plan to let
> them know and ask for help, or what's the plan with getting these things
> fixed?
I do not think they care a lot about our packaging policy.
I think for now we must try to solve the problem only for Fedora.
I do not know how long we will need to solve it, since xbean is a package not maintained by me. an update could only worsen the situation.
Comment 15 Ville Skyttä 2016-05-13 13:12:17 EDT
Looks like previous revisions of the package have been removed, the latest has been somehow updated today without a trace in %changelog and without the release bumped and the previous 1.2-1 has been overwritten at gil.fedorapeople.org. Also the upstream bug report (link in comment 9) gives a hint that maybe this package has changed to use a bundled library or something, but there's no bundled(...) Provides in the specfile.

I would have wanted to check the recent changes to the package to see the reasoning and effect behind them -- especially because the %changelog or comments here don't shed light on them. But that's not possible without the old revisions, so continuing the review would practically mean starting from scratch for me.

This combined with the fact that I don't actually have use for this package makes the whole shebang more than what I want to spend time on, so I'll bow out and let someone else -- hopefully one who'll be using the package -- finish the review.
Comment 16 gil cattaneo 2016-05-13 18:12:04 EDT
(In reply to Ville Skyttä from comment #15)
> Looks like previous revisions of the package have been removed, the latest
> has been somehow updated today without a trace in %changelog and without the
> release bumped and the previous 1.2-1 has been overwritten at
> gil.fedorapeople.org. Also the upstream bug report (link in comment 9) gives
> a hint that maybe this package has changed to use a bundled library or
> something, but there's no bundled(...) Provides in the specfile.

The package contains no library - bundle (...) -. The only significant change is in dependencies used to be able to operate: org.apache.xbean:xbean-asm-util.
which contains only the class EmptyVisitor and "extends" only org.objectweb.asm.ClassVisitor and is NOT org.objectweb.asm.commons.EmptyVisitor

> I would have wanted to check the recent changes to the package to see the
> reasoning and effect behind them -- especially because the %changelog or
> comments here don't shed light on them. But that's not possible without the
> old revisions, so continuing the review would practically mean starting from
> scratch for me.

Is NOT necessary, this package use xbean-finder-shaded which contains:
xbean-asm-util
org.ow2.asm:asm
org.ow2.asm:asm-commons
but in our xbean package, have not been "masked"

> This combined with the fact that I don't actually have use for this package
> makes the whole shebang more than what I want to spend time on, so I'll bow
> out and let someone else -- hopefully one who'll be using the package --
> finish the review.

I don't care, and thanks
Comment 17 gil cattaneo 2016-05-13 18:16:01 EDT
references and differences with the original class EmptyVisitor
http://grepcode.com/file/repo1.maven.org/maven2/asm/asm-all/3.3.1/org/objectweb/asm/commons/EmptyVisitor.java/

Note You need to log in before you can comment on or make changes to this bug.