Bug 1226926 - Review Request: eclipse-e4-importer - Alternative importer of Eclipse projects
Summary: Review Request: eclipse-e4-importer - Alternative importer of Eclipse projects
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Alexander Kurtakov
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-06-01 12:57 UTC by Sopot Cela
Modified: 2015-06-03 12:16 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-06-03 12:16:36 UTC
Type: ---
Embargoed:
akurtako: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Sopot Cela 2015-06-01 12:57:10 UTC
Spec URL: https://sopotc.fedorapeople.org/eclipse-e4-importer/eclipse-e4-importer-1.0.0-0.1.gitc0957a7.fc22.src.rpm

SRPM URL: https://sopotc.fedorapeople.org/eclipse-e4-importer/eclipse-e4-importer.spec

Description: 
The packaged Eclipse plugin provides UI entries to enable an alternative import mechanism, relying on discovery of projects (rather than user choice). More details at https://wiki.eclipse.org/E4/UI/Smart_Import .

Fedora Account System Username: sopotc

Comment 1 gil cattaneo 2015-06-01 13:18:07 UTC
hi,
first of all welcome
Suggestion for spec file
you can use also
%pom_add_plugin org.eclipse.tycho:tycho-maven-plugin:'${tycho-version}' . "<extensions>true</extensions>"
regards

Comment 2 Richard Shaw 2015-06-01 13:21:04 UTC
I'm not familiar enough with eclipse to take this review but with a drive by spec review it looks pretty good. The only thing I see missing in the files section is a license file. Is one included in the source?

If so the license macro should be used for f21+ (and I think epel7). If you plan to support older releases then you'll have to use a conditional.

Something like:
%if 0%{?rhel} || 0%{?fedora} < 21
%doc COPYING
%else
%license COPYING
%endif

And if epel7 does support the license macro then a "< 7" can be added to the rhel portion.

Comment 3 gil cattaneo 2015-06-01 13:22:47 UTC
(In reply to gil cattaneo from comment #1)
Seem you have duplicate tycho-maven-plugin entries in the main pom file ...
is not a good idea ...

Comment 4 Alexander Kurtakov 2015-06-01 13:34:15 UTC
I'll take this one. Sopot would you please apply gil's suggestion. Regarding license - eclipse.org projects usually have license injected at build time in the builds via special feature developed for the purpose (as EPL is mandated ). So this is not an issue. 
Once gil's suggestion is applied I'll do full review.

Comment 5 Sopot Cela 2015-06-01 14:11:56 UTC
Thank you for the prompt suggestoins.

(In reply to gil cattaneo from comment #3)
> (In reply to gil cattaneo from comment #1)
> Seem you have duplicate tycho-maven-plugin entries in the main pom file ...
> is not a good idea ...

Fixed: https://sopotc.fedorapeople.org/eclipse-e4-importer/eclipse-e4-importer.spec

Comment 6 gil cattaneo 2015-06-01 14:26:40 UTC
(In reply to Sopot Cela from comment #5)
> Thank you for the prompt suggestoins.
> 
> (In reply to gil cattaneo from comment #3)
> > (In reply to gil cattaneo from comment #1)
> > Seem you have duplicate tycho-maven-plugin entries in the main pom file ...
> > is not a good idea ...
> 
> Fixed:
> https://sopotc.fedorapeople.org/eclipse-e4-importer/eclipse-e4-importer.spec
Sorry but you maybe use '${tycho-version}' (with single quotation mark) otherwise you must use ${tycho-version} as global variable, because the version of this plugin maybe remain empty

Comment 7 Sopot Cela 2015-06-01 14:36:19 UTC
(In reply to gil cattaneo from comment #6)
> (In reply to Sopot Cela from comment #5)
> > Thank you for the prompt suggestoins.
> > 
> > (In reply to gil cattaneo from comment #3)
> > > (In reply to gil cattaneo from comment #1)
> > > Seem you have duplicate tycho-maven-plugin entries in the main pom file ...
> > > is not a good idea ...
> > 
> > Fixed:
> > https://sopotc.fedorapeople.org/eclipse-e4-importer/eclipse-e4-importer.spec
> Sorry but you maybe use '${tycho-version}' (with single quotation mark)
> otherwise you must use ${tycho-version} as global variable, because the
> version of this plugin maybe remain empty

I tried initially as you suggested with single quotation marks but I got: 

[INFO] Scanning for projects...
[ERROR] The build could not read 1 project -> [Help 1]
[ERROR]   
[ERROR]   The project org.eclipse.e4.ui:e4-ui-aggregator:0.17.0-SNAPSHOT (/home/rtest/rpmbuild/BUILD/org.eclipse.e4.ui-c0957a7a7d53655ecf9ae5047a94fe20de0e5d5d/pom.xml) has 1 error
[ERROR]     'build.plugins.plugin.version' for org.eclipse.tycho:tycho-maven-plugin must be a valid version but is '${tycho-version}'. @ line 10, column 18
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/ProjectBuildingException

Shall I remove the version at all since it is optional? It builds fine like that.

Comment 8 Sopot Cela 2015-06-01 14:47:09 UTC
Uploaded version with no version at all.

Comment 9 gil cattaneo 2015-06-01 15:36:24 UTC
(In reply to Sopot Cela from comment #7)
> (In reply to gil cattaneo from comment #6)
> > (In reply to Sopot Cela from comment #5)
> > > Thank you for the prompt suggestoins.
> > > 
> > > (In reply to gil cattaneo from comment #3)
> > > > (In reply to gil cattaneo from comment #1)
> > > > Seem you have duplicate tycho-maven-plugin entries in the main pom file ...
> > > > is not a good idea ...
> > > 
> > > Fixed:
> > > https://sopotc.fedorapeople.org/eclipse-e4-importer/eclipse-e4-importer.spec
> > Sorry but you maybe use '${tycho-version}' (with single quotation mark)
> > otherwise you must use ${tycho-version} as global variable, because the
> > version of this plugin maybe remain empty
> 
> I tried initially as you suggested with single quotation marks but I got: 
> 
> [INFO] Scanning for projects...
> [ERROR] The build could not read 1 project -> [Help 1]
> [ERROR]   
> [ERROR]   The project org.eclipse.e4.ui:e4-ui-aggregator:0.17.0-SNAPSHOT
> (/home/rtest/rpmbuild/BUILD/org.eclipse.e4.ui-
> c0957a7a7d53655ecf9ae5047a94fe20de0e5d5d/pom.xml) has 1 error
> [ERROR]     'build.plugins.plugin.version' for
> org.eclipse.tycho:tycho-maven-plugin must be a valid version but is
> '${tycho-version}'. @ line 10, column 18
> [ERROR] 
> [ERROR] To see the full stack trace of the errors, re-run Maven with the -e
> switch.
> [ERROR] Re-run Maven using the -X switch to enable full debug logging.
> [ERROR] 
> [ERROR] For more information about the errors and possible solutions, please
> read the following articles:
> [ERROR] [Help 1]
> http://cwiki.apache.org/confluence/display/MAVEN/ProjectBuildingException
> 
> Shall I remove the version at all since it is optional? It builds fine like
> that.
Yes is normal if in the pom file "tycho-version" is not declared (in the properties section)
Sorry, My fault i don't know how that is set pom file.
I have deceived your first draft of the modification on the same file.

Comment 10 gil cattaneo 2015-06-01 15:49:00 UTC
Last question why you set version to 1.0.0 when in the pom file is 0.17.0-SNAPSHOT ? (now, i had a look in the src archive :) ). arbitrary decision or makes sense?
in the first case you must use this notation:
Version:        0.17.0
Release:        0.1%{?dist}

* Mon Jun 01 2015 Sopot Cela <scela> - 0.17.0-0.1

Comment 12 gil cattaneo 2015-06-01 15:53:04 UTC
in this case "alphatag" should be SNAPSHOT.

Comment 13 Sopot Cela 2015-06-02 09:21:32 UTC
I updated the versions but used 0.1.0 as the version because that's the version of actual bundles (http://git.eclipse.org/c/e4/org.eclipse.e4.ui.git/tree/bundles/org.eclipse.e4.ui.importer/META-INF/MANIFEST.MF#n5 ). Also I reset the changelog.

Test-build went fine.

Location of SPEC and SRPM: https://sopotc.fedorapeople.org/eclipse-e4-importer/

Comment 15 Alexander Kurtakov 2015-06-02 09:52:05 UTC
Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed


Issues:
=======
* Rpmlint complains about description line too long. It should be less than 80. 
Otherwise package is good to go.


===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: License field in the package spec file matches the actual license.
     Note: There is no build directory. Running licensecheck on vanilla
     upstream sources. No licenses found. Please check the source files for
     licenses manually.
[x]: Package must own all directories that it creates.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 20480 bytes in 1 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: Package requires other packages for directories it uses.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

Java:
[x]: Bundled jar/class files should be removed before build
[x]: Packages have proper BuildRequires/Requires on jpackage-utils
     Note: Maven packages do not need to (Build)Require jpackage-utils. It
     is pulled in by maven-local

Maven:
[x]: If package contains pom.xml files install it (including metadata) even
     when building with ant
[x]: POM files have correct Maven mapping
[x]: Old add_to_maven_depmap macro is not being used
[x]: Packages DO NOT have Requires(post) and Requires(postun) on jpackage-
     utils for %update_maven_depmap macro
[x]: Package DOES NOT use %update_maven_depmap in %post/%postun
[x]: Packages use .mfiles file list instead of %{_datadir}/maven2/poms

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

Java:
[x]: Package uses upstream build method (ant/maven/etc.)
[x]: Packages are noarch unless they use JNI

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: eclipse-e4-importer-0.1.0-0.1.gitc0957a7.fc23.noarch.rpm
          eclipse-e4-importer-0.1.0-0.1.gitc0957a7.fc23.src.rpm
eclipse-e4-importer.noarch: E: description-line-too-long C This plugin provides UI entries to enable an alternative import mechanism, relying on discovery of projects (rather than user choice).
eclipse-e4-importer.src: E: description-line-too-long C This plugin provides UI entries to enable an alternative import mechanism, relying on discovery of projects (rather than user choice).
2 packages and 0 specfiles checked; 2 errors, 0 warnings.



Requires
--------
eclipse-e4-importer (rpmlib, GLIBC filtered):
    eclipse-platform
    java-headless
    jpackage-utils
    jsch
    osgi(org.eclipse.jdt.core)
    osgi(org.eclipse.jdt.launching)
    osgi(org.eclipse.pde.core)
    osgi(org.eclipse.pde.ui)



Provides
--------
eclipse-e4-importer:
    eclipse-e4-importer
    mvn(org.eclipse.e4.ui:e4-ui-aggregator:pom:)
    mvn(org.eclipse.e4:org.eclipse.e4.ui.importer)
    mvn(org.eclipse.e4:org.eclipse.e4.ui.importer.java)
    mvn(org.eclipse.e4:org.eclipse.e4.ui.importer.pde)
    osgi(org.eclipse.e4.ui.importer)
    osgi(org.eclipse.e4.ui.importer.java)
    osgi(org.eclipse.e4.ui.importer.pde)



Source checksums
----------------
http://git.eclipse.org/c/e4/org.eclipse.e4.ui.git/snapshot/org.eclipse.e4.ui-c0957a7a7d53655ecf9ae5047a94fe20de0e5d5d.tar.gz :
  CHECKSUM(SHA256) this package     : 97738fbac80100411fbcaa515dbf010bbf0dd902a094fdf97bba34d483fffdc8
  CHECKSUM(SHA256) upstream package : 97738fbac80100411fbcaa515dbf010bbf0dd902a094fdf97bba34d483fffdc8

Comment 17 Alexander Kurtakov 2015-06-02 10:11:37 UTC
Thanks. 
This package is APPROVED.

Comment 18 Alexander Kurtakov 2015-06-02 10:13:40 UTC
Sopot, I have sponsored you now. Please open scm request.

Comment 19 Sopot Cela 2015-06-02 12:41:40 UTC
New Package SCM Request
=======================
Package Name: eclipse-e4-importer
Short Description: Alternative importer of Eclipse projects
Upstream URL: https://www.eclipse.org/e4/
Owners: sopotc
Branches: f22 
InitialCC: eclipse-sig

Comment 20 Gwyn Ciesla 2015-06-02 13:23:08 UTC
Git done (by process-git-requests).


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