[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890

Didik Supriadi  changed:

   What|Removed |Added

  Flags||needinfo?(p...@bothner.com)
 CC||didiksupriad...@gmail.com




-- 
You are receiving this mail because:
You are always notified about changes to this product and component
You are on the CC list for the bug.
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
Fedora Code of Conduct: 
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: 
https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org
Do not reply to spam on the list, report it: 
https://pagure.io/fedora-infrastructure

needinfo canceled: [Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

Product: Fedora
Version: rawhide
Component: Package Review

Package Review  has canceled Package
Review 's request for Zbigniew
Jędrzejewski-Szmek 's needinfo:
Bug 1424890: Review Request: domterm - terminal emulator based on web
technologies
https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #35 from Package Review 
---
This is an automatic action taken by review-stats script.

The ticket reviewer failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we reset the status and the assignee of this ticket.
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
Fedora Code of Conduct: 
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: 
https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org
Do not reply to spam on the list, report it: 
https://pagure.io/fedora-infrastructure

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890

Petr Pisar  changed:

   What|Removed |Added

 CC||ppi...@redhat.com
  Flags||fedora-review?




-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
Fedora Code of Conduct: 
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: 
https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #33 from Per Bothner  ---
"Source0 is wrong. It seems you didn't actually tag 1.0.2?"

I've made a habit of updating the version number in domterm.spec when I update
the version number in the git repository itself.  It probably makes more sense
to only update the version number in domterm.spec when creating a tag or
release.

The "BSD1" is because some package-test program didn't like plain "BSD" - don't
remember which.  I'll change it to the latter.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: 
https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org/message/K6AOMLYHLTERVYFPOECX5N2EA7HJCUEM/

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #32 from Per Bothner  ---
In terms of I reviewing other packages, I'm having trouble finding packages
that haven't already gotten the kind of review I'd be qualified to give.  There
there are some packages in my areas of interest/expertise, but they already
have a number of comments.

One exception is js-viewport-navigation
(https://bugzilla.redhat.com/show_bug.cgi?id=1571742).  There I focused on the
package itself, rather than the packaging, and opined it was not suitable/ready
for Fedora - hopefully in a constructive way.

I tried fedora-review on pgcli
(https://bugzilla.redhat.comshow_bug.cgi?id=1570551) but ran into problems, as
you can see.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: 
https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org/message/PR22YA72EJVFNQKEXY2AG4JEIVGDOWUT/

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #31 from Zbigniew Jędrzejewski-Szmek  ---
Hi,

sorry for the delay.

Please in the future use real links to the spec file and rpm. fedora-review
than can download the srpm and spec automatically. In this case it should be
something like:
https://github.com/PerBothner/DomTerm/releases/download/1.0/domterm-1.0-1.fc27.src.rpm
https://raw.githubusercontent.com/PerBothner/DomTerm/master/domterm.spec

Source0 is wrong. It seems you didn't actually tag 1.0.2?

> %global version ...
This is not needed. Macro %version is automatically created from the Version
field.

> License: BSD1
The license tag should be one from
https://fedoraproject.org/wiki/Licensing:Main#Software_License_List.
I think you need License: BSD with advertising.

But please consider switching to two-clause BSD, which is compatible with GPL.

In %files, I'd suggest just useing %{_datadir}/domterm/help/ (without %dir).
Unless you really want to list all files there. This will make version upgrades
a bit easier.

Apart from this package, can you please do the reviews mentioned in
https://bugzilla.redhat.com/show_bug.cgi?id=1424890#c7. I'd like to wrap this
up ;)

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: 
https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org/message/YVRPZB3D6N36EAMB6FM6N3FD652IBC6S/

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #30 from Per Bothner  ---
I've released DomTerm 1.0 (adn announced it on LWN:
https://lwn.net/Articles/750319/)

See https://github.com/PerBothner/DomTerm/releases/tag/1.0
which includes rpms.
Specfile: https://github.com/PerBothner/DomTerm/blob/master/domterm.spec

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #29 from Per Bothner  ---
Updated to deal with the problems you noted, plus some others.
The most important is that the Source0 URL is for a release tar-ball (as
produced
by 'make dist'), rather than a git snapshot:  The former includes "missing"
files added by 'make dist'.

http://per.bothner.com/tmp/domterm-0.99.2-1.fc27.src.rpm
http://per.bothner.com/tmp/domterm.spec

fedora-review reports some weird warnings/errors, including:

domterm.x86_64: W: only-non-binary-in-usr-lib

No idea what that means.

domterm.x86_64: E: zero-length /usr/share/domterm/help/domterm-attach.txt
(and more)

I don't know what this means - after rpm -i those files are installed but
they're not zero-length.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #28 from Zbigniew Jędrzejewski-Szmek  ---
No idea. Probably best to leave it out for now.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #27 from Per Bothner  ---
Thanks for the feedback!  A question about the *.appdata.xml files.

I didn't find any real documentation on the  tag in either
https://fedoraproject.org/wiki/Packaging:AppData#app-data-validate_usage or
https://www.freedesktop.org/software/appstream/docs/sect-Metadata-Application.html#spec-appdata-filespec.
The only thing I found was
https://blogs.gnome.org/hughsie/2016/01/25/appdata-and-the-gettext-domain/

I could add:
 domterm
but that seems kind-of silly when there is no translation support at this
point.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #26 from Zbigniew Jędrzejewski-Szmek  ---
BR: gcc or g++ is also necessary now, see
https://fedoraproject.org/wiki/Changes/Remove_GCC_from_BuildRoot.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #25 from Zbigniew Jędrzejewski-Szmek  ---
Wow, I have to say that it has come a long way. I tried using both the browser
and qt versions, and they work very nicely.

Packaging looks good. One thing seems to be missing — validation of the
appstream files. See
https://fedoraproject.org/wiki/Packaging:AppData#app-data-validate_usage.

validate-relax passes, but validate does not:
$  appstream-util validate /usr/share/appdata/domterm.appdata.xml
/usr/share/appdata/qtdomterm.appdata.xml
/usr/share/appdata/domterm.appdata.xml: FAILED:
• tag-missing   :  not specified
• tag-missing   :  is not present
• style-invalid : Not enough  tags, minimum is 1
• tag-missing   :  is not present
/usr/share/appdata/qtdomterm.appdata.xml: FAILED:
• tag-missing   :  not specified
• tag-missing   :  is not present
• style-invalid : Not enough  tags, minimum is 1
• tag-missing   :  is not present
Validation of files failed
(The requirements for appdata files keep evolving. So something that was
passing a few months back might not anymore.)

Package fails to build in rawhide mock:
+ autoreconf
BUILDSTDERR: configure.ac:14: error: required file 'autotools-aux/compile' not
found
BUILDSTDERR: configure.ac:14:   'automake --add-missing' can install 'compile'
BUILDSTDERR: configure.ac:10: error: required file 'autotools-aux/missing' not
found
BUILDSTDERR: configure.ac:10:   'automake --add-missing' can install 'missing'
BUILDSTDERR: autoreconf: automake failed with exit status: 1
RPM build errors:
BUILDSTDERR: error: Bad exit status from /var/tmp/rpm-tmp.mosYgv (%build)
BUILDSTDERR: Bad exit status from /var/tmp/rpm-tmp.mosYgv (%build)
Child return code was: 1

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #24 from Per Bothner  ---
Sorry it took a while - I needed to re-write focus handling which led to a
re-write of "input line editing" http://domterm.org/Input-line-editing.html
(kind of a builtin readline library). OTOH I'm very happy with the result and I
think we can declare DomTerm 1.0 any time.  (There is always a wishlist of
things I'd like to get done, but none I think are show-stoppers - except more
testing is needed.)

http://per.bothner.com/tmp/domterm.spec
http://per.bothner.com/tmp/domterm-0.99-1.fc27.src.rpm
http://per.bothner.com/tmp/domterm-0.99-1.fc27.x86_64.rpm
http://per.bothner.com/tmp/qtdomterm-0.99-1.fc27.x86_64.rpm

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #23 from Zbigniew Jędrzejewski-Szmek  ---
Can you post the updated spec and srpm?

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #22 from Per Bothner  ---
Yes, I'm still interested. Since the previous "push" I'm made major changes
that effect its packaging, especially the qtdomterm sub-package (which now only
handles the UI), and help/man pages. Plus lots of features and polishing and
speed-up and ports (to Atom, to Theia, to Ubuntu, to Windows using WSL). I've
also posted articles to OpenSource.com and dzone.com (submitted yesterday).

I've done some updates to domterm.spec, but it needs more testing and review.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #21 from Andrew Toskin  ---
It's been a while. Per Bothner, are you still interested in working on this
package?

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #20 from Zbigniew Jędrzejewski-Szmek  ---
(In reply to Per Bothner from comment #19)
> "The built domterm package contains the directory /usr/lib/.build-id/  --
> which looks to me like leftovers from the build. Probably this isn't needed
> in the binary RPM, right?"
That's generated by rpm itself. It was added in
https://fedoraproject.org/wiki/Changes/ParallelInstallableDebuginfo. rpmlint
doen't know about this yet.

> "The spec License tag simply says "BSD". Since there are so many variants on
> the BSD license, I don't think that's specific enough."
> 
> From what I can tell, "BSD" is correct:
> https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/
> LicensingGuidelines#Valid_License_Short_Names
> https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#Good_Licenses
Yes.

> "Moreover, fedora-review's license check finds 3 files in /qtdomterm/ which
> have comments stating that they use the GNU GPL, which I think might mean
> that all of DomTerm should also use GPL(?)."
> 
> Only qtdomterm needs to be GPL.  This is only an issue with certain
> "backend" files, and I think it will make sense to rip those out in favor of
> using ldomterm, for the sake of simplicity.  But until then I changed the
> qtdomterm license to "GPLv2+".
> 
> "I also suspect that all these files containing copied code might amount to
> library bundling, which is at least discouraged in Fedora."
Bundling is allowed since 
https://pagure.io/fesco/issue/1483. Bundling is reluctantly accepted. This
makes a lot of sense especially for javascript. Most people have given up
on unbundling javascript.


The appdata file should be validated using appstream-util validate-relax
--nonet.
(If the make file does this already by itself, ignore my comment.)

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #19 from Per Bothner  ---
Updated (same locations):

Spec URL: http://per.bothner.com/DomTerm/domterm.spec
SRPM URL: http://per.bothner.com/DomTerm/domterm-0.74-1.fc25.src.rpm

I'll clean up the Source0 commans and version numbers when when everything else
is OK.  It probably makes sense until libwebsockets 2.2 is updated and I can
test against that.

".desktop files should be installed using ... desktop-file-validate"

Fixed,

"rpmlint caught an issue anyway, though: the .desktop file contains the line
`Exec=domterm`, but there is no `domterm` file under /usr/bin/ -- just ldomterm
and qtdomterm."

Well, there is be a /usr/bin/domterm installed, using alternates, but I changed
the .desktop line to Exec=ldomterm.

"The built domterm package contains the directory /usr/lib/.build-id/  -- which
looks to me like leftovers from the build. Probably this isn't needed in the
binary RPM, right?"

I'm somewhat mystified by this.  I noticed the /usr/lib/.build-id directory is
full of links on Rawhide, but doesn't exist on F25.

It seems to be related to this:
https://fedoraproject.org/wiki/Releases/FeatureBuildId

"fedora-review detects some directories without a known owner."

Fixed.

"The spec License tag simply says "BSD". Since there are so many variants on
the BSD license, I don't think that's specific enough."

From what I can tell, "BSD" is correct:
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/LicensingGuidelines#Valid_License_Short_Names
https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#Good_Licenses

"Moreover, fedora-review's license check finds 3 files in /qtdomterm/ which
have comments stating that they use the GNU GPL, which I think might mean that
all of DomTerm should also use GPL(?)."

Only qtdomterm needs to be GPL.  This is only an issue with certain "backend"
files, and I think it will make sense to rip those out in favor of using
ldomterm, for the sake of simplicity.  But until then I changed the qtdomterm
license to "GPLv2+".

"I also suspect that all these files containing copied code might amount to
library bundling, which is at least discouraged in Fedora."

Do you mean the third-party JavaScript files I use?  They're relatively small,
and I think trying to package a bunch of JavaScript client files would be a
pain.

"Right, DomTerm itself doesn't really *need* Java, it's optional for only a
couple features. ... If it would only be an issue when trying to use those
optional features, and preferably if DomTerm can gracefully handle Java's
absence, then I guess you're fine."

I think so. And I think it may be preferable to forcing installation of a bunch
of Java packages.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #18 from Andrew Toskin  ---
(In reply to Per Bothner from comment #16)
> sources file doesn't exist. Source files download skipped.

This part you can ignore for the moment. This isn't documented very well, and
I'm planning on fixing that as soon as I can set aside the time for it: After
your package is approved and you've been added to the packagers group, you can
use fedpkg to generate the `sources` file and upload the source tarballs to the
"lookaside cache" for building on the Fedora infrastructure. The warning about
the sources file is sort of pointless for a package that hasn't been approved
yet.


> error: File
> /home/bothner/DomTerm/d66241a62eb4cc9ffe90760ef39c7d890ef627a0.tar.gz: No
> such file or directory

It's funny, fedora-review will download files for you, but for fedpkg, you have
to download the source tarball before fedpkg can build with it. When the
Source0 resolves to a valid online URL, you can download the tarball with
`spectool --get-files ./domterm.spec`. You could also manually feed the tarball
URL to wget or curl, but downloading via spectool helps to make sure your RPM
spec has the correct URL.

fedpkg should build just fine after that. Doing a `mockbuild` instead of a
`local` build provides a more consistent build environment, and helps make sure
you've properly listed all dependencies. I would usually do something like
this:

  fedpkg --release f27  mockbuild --no-clean-all


...Or, this *should* work. When I use the most recently linked spec file, and
download

  https://github.com/PerBothner/DomTerm/archive/%{commit0}.tar.gz

and do a fedpkg mockbuild, it fails with this error message:

  File not found:
/builddir/build/BUILDROOT/domterm-0.74-1.fc26.x86_64/usr/share/applications/domterm.desktop

And likewise for the qtdomterm.desktop. The Makefile.am does create the
directory `$(DESTDIR)$(datadir)/applications`, but doesn't seem to actually
install the .desktop files. In fact, I didn't see any mention of "desktop" in
the Makefile at all, so I added these lines to the spec file, at the end of the
%install section.

  cp -p  domterm.desktop   
%{buildroot}/%{_datadir}/applications/domterm.desktop
  cp -p  qtdomterm.desktop 
%{buildroot}/%{_datadir}/applications/qtdomterm.desktop

The fedpkg mockbuild for DomTerm runs successfully then. The equivalent
Makefile commands might be better than appending to the spec file, though.


(In reply to Per Bothner from comment #17)
> - see this comment in the spec file:
> 
>   Java is needed for the stylesheet-manipulating subcommands of dt-util...

Right, DomTerm itself doesn't really *need* Java, it's optional for only a
couple features. But you are choosing to *build* with Java. I haven't tested
this, but I wondered if DomTerm would crash or throw errors when launching on a
system that does not have Java installed, if you're configuring it to expect
Java. If it would only be an issue when trying to use those optional features,
and preferably if DomTerm can gracefully handle Java's absence, then I guess
you're fine.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #17 from Per Bothner  ---
Concerning:

"If you're configuring to build with java, shouldn't java be a Requires instead
of a Recommends?"

- see this comment in the spec file:

  Java is needed for the stylesheet-manipulating subcommands of dt-util.
  This may change (it's just an implementation legacy).
  The domterm package also includes Java client and server classes
  that are useful for Java applications (for example Kawa).
  It is convenient to put them in the same domterm.jar as the
  much-bigger JavaScript and style files.  However, these Java classes
  are not needed for the ldomterm or qtdomterm applications.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #16 from Per Bothner  ---
Concerning setting Source0, the problem is I can't get fedpkg to work if I set
Source0 to the github download.  I'm probably invoking fedpkg incorrectly - but
I'm trying to follow the examples in
https://fedoraproject.org/wiki/How_to_create_an_RPM_package

When I set:
  Source0: https://github.com/PerBothner/DomTerm/archive/%{commit0}.tar.gz
and try to run:
  $ fedpkg --release f25 local
I get:

sources file doesn't exist. Source files download skipped.
error: File
/home/bothner/DomTerm/d66241a62eb4cc9ffe90760ef39c7d890ef627a0.tar.gz: No such
file or directory

If I set:
  Source0: 
https://github.com/PerBothner/DomTerm/archive/%{commit0}.tar.gz#/DomTerm-%{commit0}.tar.gz
I at least get the correct tar file:
sources file doesn't exist. Source files download skipped.
error: File
/home/bothner/DomTerm/DomTerm-d66241a62eb4cc9ffe90760ef39c7d890ef627a0.tar.gz:
No such file or directory

The DomTerm-d66241a62eb4cc9ffe90760ef39c7d890ef627a0.tar.gz at least matches
the filename if I open the specified URL in a browser.

Same problem with: fedpkg --release f25 mockbuild

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #15 from Andrew Toskin  ---
Your spec file still has several Source0 tags, all but one commented out.
Again, the macro-ized URL we would normally expect for a project hosted on
GitHub is this:

  https://github.com/PerBothner/DomTerm/archive/%{version}.tar.gz

One issue with this, though, is that the project's GitHub seems to be a bit
behind the source you're using to build the SRPM. You're building DomTerm 0.74,
but the latest release I see tagged on GitHub is still version 0.72.

If you do use that URL, you'd probably also have to update the directory name
used in the autosetup line (currently, `%autosetup -n DomTerm-%{commit0}`). Or
if, as the %{commit} macro suggests, you plan to build a prerelease from
arbitrary commits, then you'd use this very similar macro-ized URL:

  https://github.com/PerBothner/DomTerm/archive/%{commit0}.tar.gz

And then you probably ought to change the spec Release tag to reflect the fact
that it's a prerelease (that is, use a number like 0.1 instead of 1).


The most recent entry in the spec %changelog is a little behind, as it mentions
version 0.72-1, rather than 0.74-1. I don't think it's super important to have
a detailed spec changelog before this package has been initially approved, but
rpmlint complains when the spec Version tag and the version mentioned in the
latest changelog entry are out of sync. At least *replace* the changelog entry,
if not adding new ones --- until the package is approved. *Then* you should
keep the several most recent entries, so users can at their option check the
changelog before they install updates.


In the %build section, I noticed this line:

  %configure --disable-pty --with-qtwebengine --with-java --with-libwebsockets

If you're configuring to build with java, shouldn't java be a Requires instead
of a Recommends?


I think .desktop files should be installed using desktop-file-install and/or
desktop-file-validate, to ensure they validate.

rpmlint caught an issue anyway, though: the .desktop file contains the line
`Exec=domterm`, but there is no `domterm` file under /usr/bin/ -- just ldomterm
and qtdomterm.


The built domterm package contains the directory /usr/lib/.build-id/  -- which
looks to me like leftovers from the build. Probably this isn't needed in the
binary RPM, right?


fedora-review detects some directories without a known owner. Add macro-ized
versions of these to your %files section.

  /usr/share/domterm/defaults/
  /usr/share/domterm/defaults/preferences/


The spec License tag simply says "BSD". Since there are so many variants on the
BSD license, I don't think that's specific enough. You would need to use
"BSD-simplified" or whatever the standard abbreviation for the current license
is...

Moreover, fedora-review's license check finds 3 files in /qtdomterm/ which have
comments stating that they use the GNU GPL, which I think might mean that all
of DomTerm should also use GPL(?).

I also suspect that all these files containing copied code might amount to
library bundling, which is at least discouraged in Fedora. But I'll let
Zbigniew have the final say on this.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #14 from Per Bothner  ---
Oops, that should have been 0.74. not 0.75.  Hence:

Spec URL: http://per.bothner.com/DomTerm/domterm.spec
SRPM URL: http://per.bothner.com/DomTerm/domterm-0.74-1.fc25.src.rpm

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #13 from Andrew Toskin  ---
I can see the spec, but trying to download the SRPM returns Error 404.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #12 from Per Bothner  ---
I've updated DomTerm to take into account your comments

Spec URL: http://per.bothner.com/DomTerm/domterm.spec
SRPM URL: http://per.bothner.com/DomTerm/domterm-0.75-1.fc25.src.rpm

* It no longer uses RPM_BUILD_ROOT.
* It uses %make_install.
* I split up BuildRequires into one per dependency.
* I removed %{_bindir} in a comment.
* I added:
  Recommends: java
See the comment in the spec file.
* I added .desktop and .appdata.xml files.
* I added some more features in the %description.

You reported that the following didn't work for you:
$ ldomterm
I noticed that starting google-chome in '-app' mode doesn't
work if google-chome (as a regular browser) isn't already running.
Could you try:
$ ldomterm --firefox
and/or
$ ldomterm --browser
It might be reasonable to change the default to --firefox,
which is a "XUL" wrapper.  The big problem is that Mozilla
intends to remove XUL support this year, and I haven't found
a good replacement yet.  So perhaps --browser (uses the user's
default desktop browser) is the safest default.

There is a new dt-util script, which is meant to be used when
running in a DomTerm terminal:
http://domterm.org/dt-util.html

Note I recently implemented a major new feature, paging:
http://domterm.org/Paging.html
However, this only works with the ldomterm server, and only with
libwebsockets 2.2.  Plus the UI (including key-bindings) needs work.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890
Bug 1424890 depends on bug 1422477, which changed state.

Bug 1422477 Summary: libwebsockets-2.2.0 is available
https://bugzilla.redhat.com/show_bug.cgi?id=1422477

   What|Removed |Added

 Status|ON_QA   |CLOSED
 Resolution|--- |ERRATA



-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #11 from Zbigniew Jędrzejewski-Szmek  ---
Ooops. For some reason I assumed it's unicode.
I tested with https://www.cl.cam.ac.uk/~mgk25/ucs/examples/UTF-8-demo.txt, and
that displays beautifully.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #10 from Per Bothner  ---
Re comments #8 and #9 [Qt font problem]:

The Emacs HELLO file is not Unicode. The 'Local Variables' section of the file
says it is 'coding: iso-2022-7bit' . ISO 2022 is not terribly interesting these
days. Neither xterm nor gnome-terminal handle it, at least not by default.

"It doesn't seem to just a case of missing fonts, instead multi-byte utf8
characters are converted to some ascii noise."

That's because they're not multi-byte utf8 characters.

About the ugly menu display: I've seen similar qtdomterm font issues myself,
but they want away.  Not sure what caused it or what fixed it. :-(  I have 4k
monitor but it's 42", so it probably wouldn't count as HiDPI ...

[I'll get to the more complex comment #7 later.]

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #9 from Zbigniew Jędrzejewski-Szmek  ---
Created attachment 1267224
  --> https://bugzilla.redhat.com/attachment.cgi?id=1267224=edit
qtdomterm menu

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #8 from Zbigniew Jędrzejewski-Szmek  ---
Created attachment 1267223
  --> https://bugzilla.redhat.com/attachment.cgi?id=1267223=edit
qtdomterm screenshot

I have a HiDPI screen:
XWAYLAND0 connected 2560x1440+0+0 310mm x 170mm
   2560x1440 59.96*+
and the default fonts in qtdomterm don't work out too well.
Also note that unicode characters are not really displayed
properly. It doesn't seem to just a case of missing fonts,
instead multi-byte utf8 characters are converted to some ascii
noise.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890

Zbigniew Jędrzejewski-Szmek  changed:

   What|Removed |Added

 Status|NEW |ASSIGNED
 CC||zbys...@in.waw.pl
 Depends On||1422477
   Assignee|nob...@fedoraproject.org|zbys...@in.waw.pl



--- Comment #7 from Zbigniew Jędrzejewski-Szmek  ---
I added 1422477 as a dep, mostly to have an easy-to-access link to it. It's
almost complete anyway.

fedora-review says:
- Package uses either %{buildroot} or $RPM_BUILD_ROOT
  Note: Using both %{buildroot} and $RPM_BUILD_ROOT
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#macros

My advice is to stick to %buildroot (or %{buildroot}). That's the commonly used
modern form.
In fact, it'd be best to replace the make invocation with %make_install, which
expands to
the exact same line.

Those long lines with multiple BuildRequires should be split into
separate lines. It's not required, but in my opinion is makes it
much easier to spot mistakes and diffs look *much* better.

domterm.src:10: W: macro-in-comment %{commit0}
domterm.src:44: W: macro-in-comment %{_bindir}
→ The second one should be replaced with %%{_bindir} to avoid the warning.
The first one can stay, if you want to have a valid link in rpmspec -P output.

https://fedoraproject.org/wiki/Packaging:Java#BuildRequires_and_Requires
says that you need Requires: javapackages-tools, Requires: java.
(Either directly or indirectly, but I don't see it either way.)

Two big things which is missing, and which is strongly recommended by the
guidelines
are a .desktop file
[https://fedoraproject.org/wiki/Packaging:Guidelines#Desktop_files]
and an .appdata file [https://fedoraproject.org/wiki/Packaging:AppData].
In this case an appdata file will be particularly useful, advertising
your software and allowing potential users to see how if differers from the 
many other terminal emulators.

In the %description, please add some meat. Why is this package better, what
does it do differently than other terminal emulators, what cool features
does it sport. This text shouldn't be long, just one or two paragraphs to
pique interest. You can reuse most of the text in the appdata file anyway ;)


I built and installed the package, on F25. Seems not to work:
- in the terminal:
$ ldomterm
[9066:9066:0328/230720.355575:ERROR:child_thread_impl.cc(762)] Request for
unknown Channel-associated interface: ui::mojom::GpuMain

and there's also a big blank window which only says "Waiting for localhost..."
in
the lower left corner.

qtdomterm works OK, but it doesn't seem to support HiDPI very well (see
attached
screenshot). From what I know, it's a general problem with Qt, so I don't
expect
you to fix it. Just pointing this out in case you need stuff to improve ;)

--

I can sponsor you. My requirements apart from this package will be two or three
reviews of other packages. Your package is pretty complex, so you'll soak up a
lot of the packaging guidelines just for it, but it's always good to see and
critique what other people are doing. Please see
http://fedoraproject.org/PackageReviewStatus/NEW.html for a list of interesting
candidates. Please set up mock, if you haven't already, and base your reviews
on fedora-review output, but note that fedora-review does get stuff wrong
occasionally and leaves a lot of boxes to be filled manually. In the reviews
make a comment that your review is informal because you are not in the
packagers group yet. After you are, you can finish those reviews, if nobody
else beats you to it.

In case of questions, feel free to query me on IRC (I'm zbyszek in
#fedora-devel), or by
mail.


Referenced Bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=1422477
[Bug 1422477] libwebsockets-2.2.0 is available
-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890

Michael Schwendt  changed:

   What|Removed |Added

 Blocks||177841 (FE-NEEDSPONSOR)




Referenced Bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=177841
[Bug 177841] Tracker: Review requests from new Fedora packagers who need a
sponsor
-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #6 from Andrew Toskin  ---
(In reply to Per Bothner from comment #5)
> No, I don't have a sponsor.

Okay. If you haven't already, you'd probably want to post on the "devel"
mailing list asking for a sponsor.

 
https://lists.fedoraproject.org/archives/list/devel%40lists.fedoraproject.org/

If you *have* asked already and didn't get a response within a few days, you
can just bump the email thread. It's a busy mail list, so you have potentially
many readers, but also many other messages to compete against. So if no one
notices, just post again :)

Include a description of your package, DomTerm, and a link back to this
Bugzilla thread.


> * It would be helpful to update the libwebsockets package (to version 2.2),

Looks like updates are in the works. This thread is full of automated messages
about detecting and trying to build for 2.2, then falling back to 2.1.1.

  https://bugzilla.redhat.com/show_bug.cgi?id=1422477

If that thread closes with only a 2.1.1 build, you could manually open a ticket
about updating to 2.2.x.


> * I want domterm to install a 'dt-util' script, which does useful things
> when running under DomTerm.

Hmm, that may or may not be something you'd want or need to put in a
subpackage. I haven't needed to do anything like that yet, so I'm not sure. The
Fedora RPM Guide talks only a little about subpackages...

 
https://docs.fedoraproject.org/en-US/Fedora_Draft_Documentation/0.1/html/RPM_Guide/ch10s04.html

And there's not much in the Wiki about when to use a subpackage or not, so this
might be something to ask your future sponsor.


> * I'm thinking of including some documentation (in html form) that can be
> loaded by a hotkey.  BUt that can wait.

You've been writing your docs in Troff format so far. Especially if you're
planning on also doing HTML or other formats, you might like the idea of
writing the documentation in Markdown, or another more readable markup
language, and then converting to whatever other formats with tools like pandoc
or cmark, or other document converters.

Just a thought. I personally would to maintain a nontrivial document in Troff.


> On the other hand, I can always release a package, and update it as I
> improve it.

True :) If DomTerm is usable now, then I don't think there's much point in
waiting to package it. I mean, the timing is up to you. But you'll surely get
more and better feedback when it's easier to install the application.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #5 from Per Bothner  ---
No, I don't have a sponsor.  Thanks,

I have a few additional changes I've been working on, some of relate to
packaging:

* It would be helpful to update the libwebsockets package (to version 2.2),
though some more testing (relating to domterm) is desirable.  Once that is
done, I can remove a bunch of crud from the source code.

* I want domterm to install a 'dt-util' script, which does useful things when
running under DomTerm.  The script is written, so it mainly a matter of
updating the documentation, plus a binary spec file change (to install the
script itself and perhaps a stub man page),

* I'm thinking of including some documentation (in html form) that can be
loaded by a hotkey.  BUt that can wait.

* I'm working on an integrated pager (a la less).  It's promising, but not
quite there yet.  A lot of it is working out the UI.

On the other hand, I can always release a package, and update it as I improve
it.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #4 from Andrew Toskin  ---
@bothner
Do you have a sponsor yet? I can't sponsor you, but DomTerm seems interesting,
so I could certainly help with the basic review.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890

terrycloth  changed:

   What|Removed |Added

 CC||and...@tosk.in



--- Comment #3 from terrycloth  ---
(In reply to Per Bothner from comment #2)

> There is still some confusion between "domterm" vs "DomTerm".
> I could change the package name to DomTerm (and maybe qtDomTerm ?),
> to match the git name (on GitHub), and that might be easiest.
> However, I have no problems calling the package "domterm" if
> that doesn't cause problems.

All lower case is definitely more common for the package name and executable
file name. Your spec uses lower case everywhere except where capitalization is
hard-coded into the GitHub repo and the tarball directory, so maybe you've
already fixed this issue? `fedpkg --release f25  mockbuild` works fine for me.


> I'm also unsure how I should specify %Source0. I haven't managed to get
> working automatic downloads from GitHub - if that is actually desirable.

When you have the correct Source0 URL, it lets spectool and various other
packaging tools fetch the source tarball for you. Maybe not a huge deal if you
don't, but I think it's convenient enough to be worth using the correct macro
for the source URL. And it's actually pretty simple.

When I went to your GitHub repo's releases page, I noticed the version 0.72
release could be downloaded at this URL:

  https://github.com/PerBothner/DomTerm/archive/0.72.tar.gz

All you have to do is replace the version number -- 0.72 -- with the version
macro. Thus, the Source0 URL you want would look like this:

  https://github.com/PerBothner/DomTerm/archive/%{version}.tar.gz

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #2 from Per Bothner  ---
Thanks for the comments!  I made a number of fixes, available at:

Spec URL: http://per.bothner.com/DomTerm/domterm.spec
SRPM URL: http://per.bothner.com/DomTerm/domterm-0.72-1.fc25.src.rpm

- Fixed DomTerm to use the version number in configure.ac.

- Added %license entries to %files

- BuildRequires:  qt5-qtbase-devel qt5-qtwebchannel-devel qt5-qtwebengine-devel
  These seem to be needed; otherwise fedpkg mockbuild doesn't work.

- Used pkgconfig in BuildRequires.

- Removed 'rm -rf $RPM_BUILD_ROOT' from %install.

- Removed bogus echo command.

- Added '%dir %{_datadir}/domterm' to %files.

- Changed man page netries in %files to use ildcards.

There is still some confusion between "domterm" vs "DomTerm".
I could change the package name to DomTerm (and maybe qtDomTerm ?),
to match the git name (on GitHub), and that might be easiest.
However, I have no problems calling the package "domterm" if
that doesn't cause problems.

I'm also unsure how I should specify %Source0. I haven't managed to get working
automatic downloads from GitHub - if that is actually desirable.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1424890] Review Request: domterm - terminal emulator based on web technologies

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1424890



--- Comment #1 from Michael Schwendt  ---
> Note I had some problems with "domterm" vs "DomTerm".
> For example the command 'fedpkg --release f25 mockbuild' didn't
> work because it looks for DomTerm-0.71-1.fc25.src.rpm. 

fedpkg mockbuild works within distgit. No idea how you've used it to test-build
a separate src.rpm.

About upper-case vs. mixed cased: 
https://fedoraproject.org/wiki/Packaging:Naming#Case_Sensitivity


> Version:0.71

Build output preprocessor definitions refers to version "0.50", because that's
what configure.ac defines.


> License:BSD

https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text


> BuildRequires:  qt5-qtbase-devel qt5-qtwebchannel-devel qt5-qtwebengine-devel

Notice the following very useful "SHOULD" item in the guidelines:
https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRequires_based_on_pkg-config


> %package -n qtdomterm
> Requires:  qt5-qtbase qt5-qtwebchannel qt5-qtwebengine

https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires


> %install
> rm -rf $RPM_BUILD_ROOT

https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections


> # Let alternatives manage the symlink
> echo after install link %{buildroot}%{_bindir}/domterm

Make it a comment instead. Nobody will see this echo output.


> %files
> %{_bindir}/ldomterm
> %{_datadir}/domterm/application.ini
> %{_datadir}/domterm/chrome.manifest
> %{_datadir}/domterm/defaults/preferences/prefs.js

As long as RPM does not include directories automatically, you must specify
every directory you want to be included in the package. Either with explicit
%dir entries or by including entire directory trees:
https://fedoraproject.org/wiki/Packaging:UnownedDirectories


> %{_mandir}/man1/domterm.1.gz
> %{_mandir}/man1/ldomterm.1.gz

As rpmbuild compresses manual pages on-the-fly, changes to the global/local
configuration could change the compression program or disable compression.
That's why a growing number of packagers use wildcards to include manual pages
with any extension:

  %{_mandir}/man1/domterm.1*
  %{_mandir}/man1/ldomterm.1*

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org