Bug 1264546 - Review Request: soletta - A framework for making IoT devices
Review Request: soletta - A framework for making IoT devices
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Paulo Andrade
Fedora Extras Quality Assurance
https://github.com/solettaproject/sol...
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2015-09-18 14:42 EDT by Gustavo Lima Chaves
Modified: 2016-09-21 20:25 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-09-13 14:08:11 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
paulo.cesar.pereira.de.andrade: fedora‑review+


Attachments (Terms of Use)
v4 to v5 differences of the spec (27.98 KB, patch)
2015-10-20 15:36 EDT, Gustavo Lima Chaves
no flags Details | Diff

  None (edit)
Description Gustavo Lima Chaves 2015-09-18 14:42:59 EDT
Spec URL: https://raw.githubusercontent.com/solettaproject/soletta-packaging/v2/rpm/soletta.spec

SRPM URL: https://github.com/solettaproject/soletta-packaging/releases/download/v2/soletta-0.0.1-beta5.fc22.src.rpm

Description: Soletta project is a framework for making IoT devices. With Soletta
library developers can easily write software for devices that control
actuators/sensors and communicate using standard technologies. It
enables adding smartness even on the smallest edge devices.

Fedora Account System Username: glchaves

Sucessful koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=11137920

copr: https://copr.fedoraproject.org/coprs/glchaves/soletta/


This is my first package and I need a sponsor (fidencio@redhat.com, can you help? :)). I'm the upstream maintainer of soletta.
Comment 1 Christopher Meng 2015-09-19 05:48:13 EDT
I can't sponsor you, but I can give some suggestions.

1. Please include your email in changelog.

You can try rpmdev-bumpspec to see what exactly is changelog, obviously the current one is poor.

2. "%define soletta_major 0
%define soletta_minor 0
%define soletta_build 1
%define soletta_release beta5"

https://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define

3. Drop Group tags in all packages.

4. It's better to put %post(un/trans) after %install/%check but before %files.

5. "This package contains the sysctl linux-micro module for %{name}. The
module sets kernel parameters from sysctl.conf files. This service
will mimic systemd-sysctl.service and read the settings from
'/etc/sysctl.conf' or '/run/sysctl.d', '/etc/sysctl.d',
'/usr/local/lib/sysctl.d', '/usr/lib/sysctl.d', '/lib/sysctl.d'. Files
are processed in alphabetical order. See
http://www.freedesktop.org/software/systemd/man/systemd-sysctl.service.html.
"

Sorry, we don't use some of these paths. Please remove them.

6. The build log is silent, I could only see 

GEN *
CC *
LD *
BIN *

This makes it impossible to detect if it's been built correctly:

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

You must compile packages with %optflags and %__global_ldflags for linking, note these need to be inserted, but not substituted of all flags since you might use some custom flags as well.

7. Drop %defattr(-, root, root, -)

8. %{_includedir}/soletta/*

No, you forgot %{_includedir}/soletta/ itself, this will only include files underneath the dir without dir itself.

9. Same as above, %{_datadir}/soletta and %{_libdir}/soletta weren't included, but since different subpackages put files inside, you need to decide on your own.

10. You use %license, but put it in lib%{name}-pin-mux-module-Edison only, that's wrong, you should put it into lib%{name}. (because every subpkg depends on it based on the spec from my view)

11. I don't see any packages with name %{name}, so isn't it better to rename this spec to libsoletta?

12 Last question, do we really need such many subpackages?
Comment 2 Fabiano Fidêncio 2015-09-19 06:32:25 EDT
Christopher, thanks for jump in the review, I hope you don't mind to answer a few questions.

Why "3. Drop Group tags in all packages"? They are not the same, better have it specified, no?


Gustavo, I can be your sponsor for sure. I would like to ask you to follow the suggestions given by Christopher.
Comment 3 Jens Lody 2015-09-19 06:48:48 EDT
(In reply to Fabiano Fidêncio from comment #2)
> Christopher, thanks for jump in the review, I hope you don't mind to answer
> a few questions.
> 
> Why "3. Drop Group tags in all packages"? They are not the same, better have
> it specified, no?
> 
They are optional on Fedora and not required, only for Epel (not sure for which version), see: https://fedoraproject.org/wiki/Packaging:Guidelines#Group_tag :
- All current versions of Fedora (and their respective RPM versions) treat the Group tag as optional. Packages may include a Group: field for compatibility with EPEL, but are not required to do so.

And they litter the spec-file just more. It's hard to read anyway, because of the amount of sub-packages.

By the way:
Gustavo please fix the url's for Source0 and Source1, they do not work.
What's the cause for keeping the "config"-file locally ?
Comment 4 Gustavo Lima Chaves 2015-09-21 13:02:46 EDT
Thank you guys. I'll be fixing the pointed issues one by one today. Later on I'll resubmit, with any possible doubts posted here.
Comment 5 Gustavo Lima Chaves 2015-09-22 14:07:23 EDT
> I can't sponsor you, but I can give some suggestions.

Thanks a lot for the suggestions, Christopher! I guess Fidencio and I
will continue fine with this (thank you too, Panda!).

>
> 1. Please include your email in changelog.

Done. I really wish I could have a single, initial changelog for an
initial packaging attempt. Is that possible?

>
> You can try rpmdev-bumpspec to see what exactly is changelog, obviously the
> current one is poor.
>
> 2. "%define soletta_major 0
> %define soletta_minor 0
> %define soletta_build 1
> %define soletta_release beta5"
>
> https://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define

Thanks a lot, I won't forget.

>
> 3. Drop Group tags in all packages.

Okay, better for me.

>
> 4. It's better to put %post(un/trans) after %install/%check but before %files.

Done.

>
> 5. "This package contains the sysctl linux-micro module for %{name}. The
> module sets kernel parameters from sysctl.conf files. This service
> will mimic systemd-sysctl.service and read the settings from
> '/etc/sysctl.conf' or '/run/sysctl.d', '/etc/sysctl.d',
> '/usr/local/lib/sysctl.d', '/usr/lib/sysctl.d', '/lib/sysctl.d'. Files
> are processed in alphabetical order. See
> http://www.freedesktop.org/software/systemd/man/systemd-sysctl.service.html.
> "
>
> Sorry, we don't use some of these paths. Please remove them.

All linux-micro modules were moved to the base lib package, so these
texts are no more. I did that because linux-micro would eventually
vanish in a f23 version of the spec.

>
> 6. The build log is silent, I could only see
>
> GEN *
> CC *
> LD *
> BIN *

Well, by default our build log IS indeed silent. You can see all
that's happening with V=1, if you wish.

>
> This makes it impossible to detect if it's been built correctly:
>
> https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags
>
> You must compile packages with %optflags and %__global_ldflags for linking,
> note these need to be inserted, but not substituted of all flags since you
> might use some custom flags as well.

Nevertheless, I added those, thanks. The only "strange" output you'll
see there are cpio errors when trying to generate debug symbols for
the duktape (JavaScript) sub-module. There's an open ticket for the lib
authors to remove #line directives from their released code, that should
vanish soon (next release, maybe).

>
> 7. Drop %defattr(-, root, root, -)

Done.

>
> 8. %{_includedir}/soletta/*
>
> No, you forgot %{_includedir}/soletta/ itself, this will only include files
> underneath the dir without dir itself.

Ok, various %dir added.

>
> 9. Same as above, %{_datadir}/soletta and %{_libdir}/soletta weren't included,
> but since different subpackages put files inside, you need to decide on your
> own.

Ditto.

>
> 10. You use %license, but put it in lib%{name}-pin-mux-module-Edison only,
> that's wrong, you should put it into lib%{name}. (because every subpkg depends
> on it based on the spec from my view)

Now it happens just after the main lib's files.

>
> 11. I don't see any packages with name %{name}, so isn't it better to rename
> this spec to libsoletta?

I intend to package other things on the soletta project umbrella here.
Namely https://github.com/solettaproject/soletta-dev-app is the next
candidate, so...

>
> 12 Last question, do we really need such many subpackages?

11 of them were merged with the main lib now. The others I'd like to
keep -- they are optional for most use cases and may require different
deps.

Again, thanks a lot for the review. I'll have this new spec uploaded
soon for you to see.
Comment 6 Gustavo Lima Chaves 2015-09-22 14:10:19 EDT
> By the way:
> Gustavo please fix the url's for Source0 and Source1, they do not work.

Sorry, indeed the 0 one was wrong. What's the point of github having the downloaded file != the URL file? :/

> What's the cause for keeping the "config"-file locally ?

What's the rationale for this? We distribute all deps on external servers? This is only the configuration for Fedora systems, I thought I could skip having to address this specifically in a URL.
Comment 7 Gustavo Lima Chaves 2015-09-22 16:25:14 EDT
So, after this 1st round of reviews, here we are:

New spec URL: https://raw.githubusercontent.com/solettaproject/soletta-packaging/v3/rpm/soletta.spec

New SRPM URL: https://github.com/solettaproject/soletta-packaging/releases/download/v3/soletta-0.0.1-beta5.fc22.src.rpm

Successful koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=11182749

Successful copr build: https://copr.fedoraproject.org/coprs/glchaves/soletta/build/117590/

Please let me know if I forgot anything from the reviews.
Comment 8 Piotr Popieluch 2015-09-24 07:51:30 EDT
> Sorry, indeed the 0 one was wrong. What's the point of github having the 
> downloaded file != the URL file? :/

"spectool -g x.spec" downloads the sources. 

Most reviewers also use "fedora-review -b 1264546" for reviews, this also checks the source urls. 

fedora-review fails to build, could you please run fedora-review yourself first and fix the issues.

There are also guidelines on the Source, please see this section for the source packaging guidelines:
https://fedoraproject.org/wiki/Packaging:SourceURL?rd=Packaging/SourceURL#Git_Hosting_Services

I don't fully understand the release of the package. Release: tag should specifies the  release of the package, eg. you update the specfile and should increment the release by one. It seems to me that you filled in upstreams release, which should be part of version.
Comment 9 Piotr Popieluch 2015-09-24 08:01:15 EDT
The beta5 can also be problematic as this is not numerical and rpm won't be able to compare with newer version. Please see guidelines:
https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Pre-Release_packages
Comment 10 Gustavo Lima Chaves 2015-09-24 10:50:41 EDT
Thanks a lot, Piotr, I'll be looking at the issues you pointed today. BTW, since Fidencio can't be my sponsor (he seems not to classify for that), would you? :)
Comment 11 Piotr Popieluch 2015-09-24 13:25:05 EDT
I'm sorry but am not a sponsor either.

Best way to get sponsored fast is to do a lot of informal package reviews so you will show you understand the package guidelines. Then link to the reviews from this bug. Potential sponsors will notice the reviews.

Other option is to find a packager willing to maintain this package, you could then become co-maintainer without going through the sponsoring process.
Comment 12 Gustavo Lima Chaves 2015-09-24 13:54:33 EDT
Thanks, Piotr.

> fedora-review fails to build, could you please run fedora-review yourself first > and fix the issues.

I builds fine here. Would it be that I'm under f22 and you're running a > version? Note that I target f22 in this bug. I'll eventually put conditionals for exchanging Soletta's config file for fedoras >= 23.

Let's see if you or someone can help with this fedora-review tool's output, though (just manual stuff, I got no ! entries).

[ is this true or moot? ]: Package must own all directories that it creates.
     Note: Directories without known owners:
     /usr/share/soletta/flow/descriptions, /usr/share/soletta/flow

Now rpmlint:

libsoletta.x86_64: E: non-standard-executable-perm /usr/lib64/soletta/modules/linux-micro/network-up.so 775

I'm sure that under a virtual machine I get this file listed as 755 after installation. Is that any difference of umask during the tool's processing or what?

> The beta5 can also be problematic as this is not numerical and rpm won't be able to compare with newer version. Please see guidelines:

Mm, thanks. I'm now with the following schema, as read on the guidelines:

%global soletta_major 0
%global soletta_minor 0
%global soletta_build 1
%global soletta_tag beta5

%global soletta_duktape_tag beta2

Summary: A framework for making IoT devices
Name: soletta
Version: %{soletta_major}.%{soletta_minor}.%{soletta_build}
Release: 0.1.%{soletta_tag}%{?dist}

> There are also guidelines on the Source, please see this section for the source packaging guidelines:

Fixed, thanks a lot.
Comment 13 Michael Schwendt 2015-09-24 14:15:46 EDT
You are supposed to test-build against Rawhide and review for Rawhide.


> [ is this true or moot? ]:

https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership
https://fedoraproject.org/wiki/Packaging:UnownedDirectories

The fedora-review tool is pretty good at finding so-called "unowned directories". If you want to verify such issues, run "rpmls … | grep ^d" on the built package and check that every directory is included. As the next step, review the subpackage dependency chain and verify that the packages providing those directory entries are pulled in.

Skimming over the spec file, it seems you have the directory ownership for various dirs in the -devel package, but the many subpackages don't depend on the -devel package. Based on that I suggest you move the directory ownership or create a -common or -filesystem subpackage (as per the linked guidelines, see the yellow box).
Comment 14 Piotr Popieluch 2015-09-24 14:20:12 EDT
Version lgtm now.

See my build logs here:
https://gist.github.com/piotr1212/a8ed3ffc34bba9c88794

Package reviews should be against rawhide (f24 currently). Please check if your mock config points to rawhide:

➜ ls -l /etc/mock/default.cfg 
lrwxrwxrwx. 1 root root 35 Aug 28 21:18 /etc/mock/default.cfg -> /etc/mock/fedora-rawhide-x86_64.cfg




warning: bogus date in %changelog: Tue Sep 2 2015 Gustavo Lima Chaves <gustavo.lima.chaves@intel.com> - 0.0.1-beta5

This one should be easy to fix. Just add an space between "Sep" and "2", or change it to "Sep 02" if you prefer that notation.
Comment 15 Gustavo Lima Chaves 2015-09-24 14:32:38 EDT
Thanks, guys. I'm working on the spec to test it against rawhide too. It should not take long to post the update stuff here.
Comment 16 Michael Schwendt 2015-09-24 16:00:51 EDT
> warning: bogus date in %changelog

> Just add an space between "Sep" and "2", or change it to "Sep 02" if
> you prefer that notation.

That's *not* what the warning is about. "Tue Sep 2" is bogus, because either it's "Wed Sep 2" or "Tue Sep 1". Watch this:

$ date -d 'Tue Sep 2 2015'
Wed Sep  2 00:00:00 CEST 2015
Comment 17 Upstream Release Monitoring 2015-09-24 16:12:42 EDT
glchaves's scratch build of soletta-0.0.1-0.1.beta5.fc22.src.rpm for rawhide failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11215731
Comment 18 Gustavo Lima Chaves 2015-09-24 16:15:03 EDT
> That's *not* what the warning is about. "Tue Sep 2" is bogus, because either it's "Wed Sep 2" or "Tue Sep 1". Watch this:

Yes, Michael, I knew that and had a local fix already. BTW, I suppose it's OK to have the 1st spec in with just an initial changelog entry, right?
Comment 19 Upstream Release Monitoring 2015-09-24 16:31:04 EDT
glchaves's scratch build of soletta-0.0.1-0.1.beta5.fc22.src.rpm for rawhide completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11215899
Comment 21 Gustavo Lima Chaves 2015-09-28 10:25:29 EDT
BTW, would you sponsor me, Michael? Thanks for the tips so far :)
Comment 22 Gustavo Lima Chaves 2015-09-30 16:51:03 EDT
We released beta6 now, it should not take long till I update the specs accordingly. We're approaching the actual first release, too.
Comment 23 Upstream Release Monitoring 2015-10-16 16:20:35 EDT
glchaves's scratch build of soletta-0.0.1-0.1.beta7.fc22.src.rpm for rawhide failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11478871
Comment 24 Upstream Release Monitoring 2015-10-16 16:39:18 EDT
glchaves's scratch build of soletta-0.0.1-0.1.beta7.fc22.src.rpm for rawhide failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11479077
Comment 25 Gustavo Lima Chaves 2015-10-16 17:30:31 EDT
The second build just above failed because it now include make checks and it fails on i686. I'll fix that and re-submit.
Comment 26 Paulo Andrade 2015-10-19 00:13:07 EDT
Hi Gustavo,
We talked briefly in Latinoware, and I understand you want
this package in Fedora :) And I am sure it would be very
useful. I do not know if you want to package other software,
otherwise, an easier plan would be to approach a packager,
that would package it for you. So, at first I would like
to know if you only plan to package solleta for Fedora :)

I believe the package is in good shape, but I would like
a reply for the issues below.


---8<---
I suggest just using solleta for the package, no need for lib%{name},
unless you can give a good reason for that, in which case, should
rename the package to libsolleta. For example, instead of

%package -n lib%{name}-flow-module-accelerometer

have

%package flow-module-accelerometer


---8<---
Please put version information in the Version tag, not in the
Release tag, and bump Release every time you make an update.
For example, instead of

Version: %{soletta_major}.%{soletta_minor}.%{soletta_build}
Release: 0.1.%{soletta_tag}%{?dist}

have

Version: %{soletta_major}.%{soletta_minor}.%{soletta_build}.%{soletta_tag}
Release: 1%{?dist}

---8<---
The license should be "BSD and MIT". And I am afraid probably actually
be "BSD and MIT and GPLv2+", due to files under tools/kconfig/lxdialog
The files are not part of runtime, but are in the tarball. duktape
files are under MIT license.


---8<---
Please explain what did you mean with the line
%{?%{name}_debug_package}
in the spec file.


---8<---
Also please explain a bit about linux-micro in fedora < 23, and
systemd in newer fedora. Did you test it in newer Fedora? I suppose
you tested the package, interacting with real hardware on f22 :)


---8<---
You do not need to write

%dir %{_includedir}/soletta
%{_includedir}/soletta/*

Just write

%{_includedir}/soletta/

The ending / will make it clear is is a directory (and contents).


---8<---
Kind of related to above, you should move ownership of base
directories to a package required by all subpackages, for example,
instead of having in -devel this:

%dir %{_datadir}/soletta/flow/descriptions
...
%dir %{_datadir}/soletta/flow/descriptions/...

make the main package owner of the directory, with:

%dir %{_datadir}/soletta/flow/descriptions


---8<---
From reading above, you should be working on it, but I see
there are several test subdirectories, but the latest spec
does not have a %check section. You should run the tests, and
give explanations (can be just a comment in the spec) where it
is expected to fail.


---8<---
You should fix the wrong permissions. Better to find out form
where they are coming. Likely from CP="cp -p". In the worst
case, just run something like
find %{buildroot} -perm 0775 | xargs chmod 0755
in %install
Comment 27 Gustavo Lima Chaves 2015-10-19 11:42:33 EDT
> Hi Gustavo,

Hi, Paulo!

> We talked briefly in Latinoware, and I understand you want
> this package in Fedora :) And I am sure it would be very
> useful. I do not know if you want to package other software,
> otherwise, an easier plan would be to approach a packager,
> that would package it for you. So, at first I would like
> to know if you only plan to package solleta for Fedora :)
>
> I believe the package is in good shape, but I would like
> a reply for the issues below.

We want to start packaging the soletta framework (library, devel
headers and tools), but soon we'd also like to package another
part of the framework -- Soletta Dev
App (https://github.com/solettaproject/soletta-dev-app). That's a
web-based environment that sits on the target board and exposes
a web page where a developer can interact with soletta in a visual
way to test his/her flow-based programming programs.

No hurry, though, I'll start and get it right with Soletta solely
first. It would be cool, though, to have it all in the same spec.
Is that your concern, we'd have to start with all in there right
away?

>
>
> ---8<---
> I suggest just using solleta for the package, no need for lib%{name},
> unless you can give a good reason for that, in which case, should
> rename the package to libsolleta. For example, instead of
>
> %package -n lib%{name}-flow-module-accelerometer
>
> have
>
> %package flow-module-accelerometer

That's already done locally here, as I got the same feedback from
some folks at IRC.

>
>
> ---8<---
> Please put version information in the Version tag, not in the
> Release tag, and bump Release every time you make an update.
> For example, instead of
>
> Version: %{soletta_major}.%{soletta_minor}.%{soletta_build}
> Release: 0.1.%{soletta_tag}%{?dist}
>
> have
>
> Version: %{soletta_major}.%{soletta_minor}.%{soletta_build}.%{soletta_tag}
> Release: 1%{?dist}

Okay. Do we make updates and changelog bumps here in process of
package review too (before any publishment)? That's a doubt I had
before and no formal answer came yet.

>
> ---8<---
> The license should be "BSD and MIT". And I am afraid probably actually
> be "BSD and MIT and GPLv2+", due to files under tools/kconfig/lxdialog
> The files are not part of runtime, but are in the tarball. duktape
> files are under MIT license.

Fair, will do.

>
>
> ---8<---
> Please explain what did you mean with the line
> %{?%{name}_debug_package}
> in the spec file.

I guess I can remove that, as the -debug package is built
anyway (I just wanted to make sure it's generated).

>
>
> ---8<---
> Also please explain a bit about linux-micro in fedora < 23, and
> systemd in newer fedora. Did you test it in newer Fedora? I suppose
> you tested the package, interacting with real hardware on f22 :)
>

I did test it reasonably on f22 and I'll start testing it harder on
f23, don't worry. To explain the linux-micro saga, I'll translate
what I said to Hélio (hcastro@redhat.com) at #soletta:

Soletta has a platform module with 2 possible backends: systemd
on linux-micro (the latter being targeted to smaller linux
systems with no systemd, in which case it tries to emulate some
of the things systemd would provide). The thing is we only detect
we're on a systemd-capable system if systemd's version is at
least 221 (I think we did that for sd-bus API usage we rely on).
Thus, that decision -- when packaging for f23+, we put systemd
as our platform, otherwise, we get linux-micro.

Now, linux-micro is separated in modules, too, because a user
might want not to have all the platform functionality we provide.
That's why it has a couple of packages just for it, as well.

>
> ---8<---
> You do not need to write
>
> %dir %{_includedir}/soletta
> %{_includedir}/soletta/*
>
> Just write
>
> %{_includedir}/soletta/
>
> The ending / will make it clear is is a directory (and contents).

Fair, thanks.

>
>
> ---8<---
> Kind of related to above, you should move ownership of base
> directories to a package required by all subpackages, for example,
> instead of having in -devel this:
>
> %dir %{_datadir}/soletta/flow/descriptions
> ...
> %dir %{_datadir}/soletta/flow/descriptions/...
>
> make the main package owner of the directory, with:
>
> %dir %{_datadir}/soletta/flow/descriptions
>

Okay.

>
> ---8<---
> From reading above, you should be working on it, but I see
> there are several test subdirectories, but the latest spec
> does not have a %check section. You should run the tests, and
> give explanations (can be just a comment in the spec) where it
> is expected to fail.

What I mentioned was WIP, that's why it's not published here yet.
I decided to answer you first, but soon I'll land the updated
SRPM and stuff, right?

>
>
> ---8<---
> You should fix the wrong permissions. Better to find out form
> where they are coming. Likely from CP="cp -p". In the worst
> case, just run something like
> find %{buildroot} -perm 0775 | xargs chmod 0755
> in %install

Mm, okay. Can I ask which of the tools gave you the output so I
can trace it here too? I'll exercize all of them, just in case.

Thanks a lot for you support!
Comment 28 Michael Schwendt 2015-10-20 05:45:46 EDT
> Do we make updates and changelog bumps here in process of
> package review too (before any publishment)?

Yes.  It's only pointed out at

  https://fedoraproject.org/wiki/FrequentlyMadePackagingMistakes  

because for the review process it has been assumed that modifying a src.rpm implies changing the spec file and publishing the changes as a _new_ src.rpm release.

Occasionally, during review somebody skips the Release bump, though, and that makes reviewing less convenient (with the tools from "rpm -ql rpmdevtools|grep diff" in mind, for example).


> %{_includedir}/soletta/
> 
> The ending / will make it clear is is a directory (and contents).

As explained in the guidelines, the trailing slash is purely cosmetic. The lines

  %{_includedir}/soletta
and
  %{_includedir}/soletta/

achieve the same, regardless of whether "soletta" is a file or a directory tree. The trailing slash only increases readability of the spec file, making it explicit that the %files line doesn't refer to a single file.
Comment 29 Gustavo Lima Chaves 2015-10-20 08:51:16 EDT
> Yes.  It's only pointed out at
>
>   https://fedoraproject.org/wiki/FrequentlyMadePackagingMistakes
>
> because for the review process it has been assumed that modifying a src.rpm implies changing the spec file and publishing the changes as a _new_ src.rpm release.
>
> Occasionally, during review somebody skips the Release bump, though, and that makes reviewing less convenient (with the tools from "rpm -ql rpmdevtools|grep diff" in mind, for example).

Thanks a lot, Michael! Sorry for missing this FAQ. Now, with the
version string hopefully done right, I'll also do a Release bump.
Comment 30 Upstream Release Monitoring 2015-10-20 13:39:53 EDT
glchaves's scratch build of soletta-0.0.1.beta8-1.fc22.src.rpm for rawhide failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11518781
Comment 31 Paulo Andrade 2015-10-20 13:48:33 EDT
Hi Gustavo,

  My concern when asking if you plan to package other software is
that sometimes, getting sponsored may not be a so easy task :)
and sometimes one could have just one package in Fedora, and forget
about it.
  People usually starts with a smaller package, and participate
a lot on informal reviews, to show knowledge of the work flow
and how to create good quality rpm packages;  what an upstream
developer may not have enough time to do.



About the permissions issue, I did run:

$ fedora-review -b 1264546

on this setup:

$ ll /etc/mock/default.cfg 
lrwxrwxrwx 1 root root 25 Oct  4  2014 /etc/mock/default.cfg -> fedora-rawhide-x86_64.cfg

and my computer is also running rawhide (as of 2 weeks ago).
You also saw it in #c12
"""
libsoletta.x86_64: E: non-standard-executable-perm /usr/lib64/soletta/modules/linux-micro/network-up.so 775
"""
I did a quick search, looks like mock uses an umask to group
writable files since
https://git.fedorahosted.org/cgit/mock.git/commit/?id=6533774ebda67addfb6e701246f3a96c5e62bcd9
The install command by default does not give group write
 permission, while "cp -p" copies it exactly.



Once checking the next srpm, with %check enabled, subpackages
renamed, etc, I can assign this bug to myself, and do at least
one more review.
Unless someone else wants to be your sponsor :)
Comment 32 Gustavo Lima Chaves 2015-10-20 15:33:34 EDT
> Hi Gustavo,

Hi, Paulo.

>
>   My concern when asking if you plan to package other software is
> that sometimes, getting sponsored may not be a so easy task :)
> and sometimes one could have just one package in Fedora, and forget
> about it.
>   People usually starts with a smaller package, and participate
> a lot on informal reviews, to show knowledge of the work flow
> and how to create good quality rpm packages;  what an upstream
> developer may not have enough time to do.

I indeed have a lot of my time spent on development, but my
intention is to be responsible for the package at all times,
naturally, coming in pronto to fix any issue the arise and always
doing updates. I can try better to help on reviewing other
packages too, but being a full time packager is not possible for
me :(. Is that a blocker?

>
>
>
> About the permissions issue, I did run:
>
> $ fedora-review -b 1264546
>
> on this setup:
>
> $ ll /etc/mock/default.cfg
> lrwxrwxrwx 1 root root 25 Oct  4  2014 /etc/mock/default.cfg -> fedora-rawhide-x86_64.cfg
>
> and my computer is also running rawhide (as of 2 weeks ago).
> You also saw it in #c12
> """
> libsoletta.x86_64: E: non-standard-executable-perm /usr/lib64/soletta/modules/linux-micro/network-up.so 775
> """
> I did a quick search, looks like mock uses an umask to group
> writable files since
> https://git.fedorahosted.org/cgit/mock.git/commit/?id=6533774ebda67addfb6e701246f3a96c5e62bcd9
> The install command by default does not give group write
>  permission, while "cp -p" copies it exactly.

I think the issue is now gone with the removal of the "cp -p"
part.

>
>
>
> Once checking the next srpm, with %check enabled, subpackages
> renamed, etc, I can assign this bug to myself, and do at least
> one more review.
> Unless someone else wants to be your sponsor :)

Nice! I hope you did not step back on the idea of sponsoring
us :)

Now for the status update. We're now o beta8, where I fixed so
check issues we were having. The thing is I could not advance we
would have issues in the arm build (now getting an arm VM to
investigate better). Also, copr's kernel does not have
crypto_user support, what we used in the tests, too. beta9 tag
was rolled for that -- openssl backend is now the default for
message digest in Soletta. Besides the issues on the arm build,
here go what we got now, so at least the rest of the SRPM is
reviewed:

New spec URL: https://raw.githubusercontent.com/solettaproject/soletta-packaging/v5/rpm/soletta.spec

New SRPM URL: https://github.com/solettaproject/soletta-packaging/releases/download/v5/soletta-0.0.1.beta8-1.fc22.src.rpm

koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=11518781

I wish to keep, for now, the sole changelog line (before we get
this right), and because the version changed, the release is
still 1. I'll add an attachment with the diff from v4 to v5 of
the spec.
Comment 33 Gustavo Lima Chaves 2015-10-20 15:36 EDT
Created attachment 1084917 [details]
v4 to v5 differences of the spec

This is to aid in the changes from v4 to v5. If this is not needed, please let me know, for future changes.
Comment 34 Paulo Andrade 2015-10-22 12:30:50 EDT
Hi Gustavo,

I started the process to become your sponsor.

I believe you showed that you will have enough persistence
to maintain the package, and add new packages to Fedora :)

You should update the changelog as if the package were already
in Fedora.

I will review the new package, and let you know if anything
else is required before the first official Fedora package :)

The first issue I see in a quick look is that you did

%package %{name}-subpackage

it should have been

%package subpackage

now you have packages named:

soletta-soletta-subpackage

When not using -n, "%{name}-" is prepended to the package
name.
Comment 35 Paulo Andrade 2015-10-22 15:59:26 EDT
Hi Gustavo,

---%<---
First we will need to sort out some issues with the third party
software.

I see there is a review request for tinycbor at
https://bugzilla.redhat.com/show_bug.cgi?id=1269001
It is the same thing right?

About duktape I do not see any review or existing package.

Could tinycbor and/or duktape be used as external dependencies?
If not, what is the reason?

Note that Fedora is in the process of having more relaxed rules
for bundled packages, but it should only be used if there is no
other option.


---%<---
Another issue I noticed, is that %install appears to redo all
compilations, after regenerating header files. Some issue with
make rules?


---%<---
There are some duplicated files. I think you misunderstood some
details of adding a trailing "/". If listing only the directory,
must do it as:

%dir %{_libdir}/soletta/modules/

otherwise, there will be duplicates, because when listing a
directory it understands the directory and its contents, for
example:

%{_libdir}/soletta/modules/
%{_libdir}/soletta/modules/flow/

the line %{_libdir}/soletta/modules/ already includes
%{_libdir}/soletta/modules/flow/

Another issue is the line:

%{_includedir}/soletta/*

it should have been:

%{_includedir}/soletta/

otherwise, the directory %{_includedir}/soletta/ will not
have a owner package.


---8<---
Please comment about the warning:

In function '__fread_alias',
    inlined from 'sol_memmap_write_raw_do.isra.2' at ./src/lib/io/sol-memmap-storage.c:169:15:
/usr/include/bits/stdio2.h:290:9: warning: call to '__fread_chk_warn' declared with attribute warning: fread called with bigger size * nmemb than length of destination buffer
  return __fread_chk (__ptr, __bos0 (__ptr), __size, __n, __stream);

It is caused by the fread in this code:
    if (mask) {
        uint64_t value = 0, old_value;
        uint32_t i, j;

        for (i = 0, j = 0; i < entry->size; i++, j += 8)
            value |= (uint64_t)((uint8_t *)buffer->data)[i] << j;

        ret = fread(&old_value, entry->size, 1, file);

It looks suspicious. Shouldn't old_value be declared
as size_t, and instead of entry->size, write sizeof(old_value)
or sizeof(size_t) ? If the size is guaranteed to fit, it will
have issues in big endian architectures.


---8<---
Also fix the %package %{name}- issue.


---8<---
For the next review, please also update the changelog. This also
serves as a way to follow the history of the package :)
Comment 36 Gustavo Lima Chaves 2015-10-22 16:50:00 EDT
> Hi Gustavo,

Hi, Paulo.

>
> I started the process to become your sponsor.
>
> I believe you showed that you will have enough persistence
> to maintain the package, and add new packages to Fedora :)

Thank you so much! Yes, I'll do my best at all times, don't
worry.

>
> You should update the changelog as if the package were already
> in Fedora.

Ok, from now on, I'll do that.

>
> I will review the new package, and let you know if anything
> else is required before the first official Fedora package :)
>
> The first issue I see in a quick look is that you did
>
> %package %{name}-subpackage
>
> it should have been
>
> %package subpackage
>
> now you have packages named:
>
> soletta-soletta-subpackage
>
> When not using -n, "%{name}-" is prepended to the package
> name.

Mmm, fine. I'll fix it.

>
> Hi Gustavo,
>
> ---%<---
> First we will need to sort out some issues with the third party
> software.
>
> I see there is a review request for tinycbor at
> https://bugzilla.redhat.com/show_bug.cgi?id=1269001
> It is the same thing right?

Yes, it is. As soon as they get in, we can change to depend and
link to it.

>
> About duktape I do not see any review or existing package.
>
> Could tinycbor and/or duktape be used as external dependencies?
> If not, what is the reason?

Duktape specifically can't, as they only distribute the sources
at will, to be bundled by user projects.

>
> Note that Fedora is in the process of having more relaxed rules
> for bundled packages, but it should only be used if there is no
> other option.
>
>
> ---%<---
> Another issue I noticed, is that %install appears to redo all
> compilations, after regenerating header files. Some issue with
> make rules?

Yes, some issue on our side, it's on my TODO-list.

>
>
> ---%<---
> There are some duplicated files. I think you misunderstood some
> details of adding a trailing "/". If listing only the directory,
> must do it as:
>
> %dir %{_libdir}/soletta/modules/
>
> otherwise, there will be duplicates, because when listing a
> directory it understands the directory and its contents, for
> example:
>
> %{_libdir}/soletta/modules/
> %{_libdir}/soletta/modules/flow/
>
> the line %{_libdir}/soletta/modules/ already includes
> %{_libdir}/soletta/modules/flow/
>
> Another issue is the line:
>
> %{_includedir}/soletta/*
>
> it should have been:
>
> %{_includedir}/soletta/
>
> otherwise, the directory %{_includedir}/soletta/ will not
> have a owner package.

Sorry for the mistakes, I'll have those fixed.

>
>
> ---8<---
> Please comment about the warning:
>
> In function '__fread_alias',
>     inlined from 'sol_memmap_write_raw_do.isra.2' at ./src/lib/io/sol-memmap-storage.c:169:15:
> /usr/include/bits/stdio2.h:290:9: warning: call to '__fread_chk_warn' declared with attribute warning: fread called with bigger size * nmemb than length of destination buffer
>   return __fread_chk (__ptr, __bos0 (__ptr), __size, __n, __stream);
>
> It is caused by the fread in this code:
>     if (mask) {
>         uint64_t value = 0, old_value;
>         uint32_t i, j;
>
>         for (i = 0, j = 0; i < entry->size; i++, j += 8)
>             value |= (uint64_t)((uint8_t *)buffer->data)[i] << j;
>
>         ret = fread(&old_value, entry->size, 1, file);
>
> It looks suspicious. Shouldn't old_value be declared
> as size_t, and instead of entry->size, write sizeof(old_value)
> or sizeof(size_t) ? If the size is guaranteed to fit, it will
> have issues in big endian architectures.

I saw it too, it won't be there anymore on the next beta release.
Thanks.

>
>
> ---8<---
> Also fix the %package %{name}- issue.
>
>

Will do.

> ---8<---
> For the next review, please also update the changelog. This also
> serves as a way to follow the history of the package :)

Okay. It may take me a few time for the next update, since I want
to sort that arm bot error first. I guess I'll try it on a
raspberry-pi fedora image here, unless I can get access to some
of the community arm machines :/

Thanks a lot for the review and sponsoring again :)
Comment 37 Paulo Andrade 2015-10-23 20:23:53 EDT
Hi Gustavo,

In the next package you submit for review, please add this
to the main package:

Provides:	bundled(tinycbor) = 0.2
Provides:	duktape-static = 1.2.2

So that it can be tracked with a query of the packages.

The bundled(tinycbor) is to remember it has a bundled
library that may be updated.

The duktape-static is due to the policy of when a package
is composed only of header files, or in this case, also
of a .c file.
Comment 38 Michael Schwendt 2015-10-24 07:21:58 EDT
> Provides:	duktape-static = 1.2.2

Unusual. Those -static Provides are added to packages, which are build requirements themselves, *not* to packages that are linked statically.

If a static version of duktape is linked with *and* bundled with the sources, just add

  Provides:	bundled(duktape) = 1.2.2

to the package.


> So that it can be tracked with a query of the packages.

Such a query examines which -static packages appear in BuildRequires actually.
Comment 39 Paulo Andrade 2015-10-26 22:28:56 EDT
  Thanks Micahel.
  Gustavo, please follow Michael advice, and add
Provides:	bundled(duktape) = 1.2.2
duktape-static should be provided by a duktape package,
that would probably consist of two files, named:
%{_includedir}/duktape/duktape.{c,h}
plus license and any doc files, and can be a very
simple, next package for you :)
Then, later you can make soletta "BuildRequires: duktape-static"
Comment 40 Upstream Release Monitoring 2015-10-27 09:34:50 EDT
glchaves's scratch build of soletta-0.0.1.beta10-1.fc22.src.rpm for rawhide completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11598812
Comment 41 Gustavo Lima Chaves 2015-10-27 09:42:39 EDT
> In function '__fread_alias',
>     inlined from 'sol_memmap_write_raw_do.isra.2' at ./src/lib/io/sol-memmap-storage.c:169:15:
> /usr/include/bits/stdio2.h:290:9: warning: call to '__fread_chk_warn' declared with attribute warning: fread called with bigger size * nmemb than length of destination buffer
>   return __fread_chk (__ptr, __bos0 (__ptr), __size, __n, __stream);
>
> It is caused by the fread in this code:
>     if (mask) {
>         uint64_t value = 0, old_value;
>         uint32_t i, j;
>
>         for (i = 0, j = 0; i < entry->size; i++, j += 8)
>             value |= (uint64_t)((uint8_t *)buffer->data)[i] << j;
>
>         ret = fread(&old_value, entry->size, 1, file);
>
> It looks suspicious. Shouldn't old_value be declared
> as size_t, and instead of entry->size, write sizeof(old_value)
> or sizeof(size_t) ? If the size is guaranteed to fit, it will
> have issues in big endian architectures.

I took a closer look and we're out of ideas on how to make gcc to shut up there. The assert() there was put in by the respective dev to make sure we got that space (we're under an if clause that guarantees that). I'll leave that as is for now.

> ---%<---
> Another issue I noticed, is that %install appears to redo all
> compilations, after regenerating header files. Some issue with
> make rules?

This one will be my next pursuit, ok?

>  Gustavo, please follow Michael advice, and add
> Provides:	bundled(duktape) = 1.2.2
> duktape-static should be provided by a duktape package,
> that would probably consist of two files, named:
> %{_includedir}/duktape/duktape.{c,h}
> plus license and any doc files, and can be a very
> simple, next package for you :)
> Then, later you can make soletta "BuildRequires: duktape-static"

Ok, I did like he advised, thanks. I can talk to duktape folks later,
sure.

All other review points were already adressed.
Comment 43 Michael Schwendt 2015-10-27 18:22:49 EDT
Hard to tell, as entry->size could be anything and isn't checked within that function.

Can you enhance the build, so that the output is more verbose and shows the detailed invocations of gcc and ld? That would be the only way to tell how exactly the files are compiled/linked.

Who knows under which circumstances results like https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=751812 turn up with GCC 5?


> Version: %{soletta_major}.%{soletta_minor}.%{soletta_build}.%{soletta_tag}

Hard to follow why it's done like that. As it ends up with

  soletta-0.0.1.beta10-1.fc22.src.rpm

that doesn't follow the Package Versioning Guidelines:

  https://fedoraproject.org/wiki/Packaging:Guidelines#Version_and_Release
  https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Package_Versioning

Those would move the "beta10" out of %version and into a less-significant position in %release. That can be important for all cases where RPM version comparison is not compatible with upstream's release versioning scheme.
Comment 44 Gustavo Lima Chaves 2015-10-28 09:32:47 EDT
Thanks, Michael. I agree, and if you check the history it was like the guidelines before Paulo asked me to change (Release: 0.1.%{soletta_tag}%{?dist}). Paulo, do you agree that we must revert to that?

As for the entry->size thing, I'll double check again. I'll also add V=1 to our make invocation, so you guys see the exact tool invocation lines.
Comment 45 Paulo Andrade 2015-10-28 12:41:56 EDT
Hi Gustavo and Michael.

I just started a new fedora-review call, so will comment about
other issues later.

About versioning in release. The guidelines tell how to handle
in a way that has less chances of having issues when upstream
and rpm do not agree on versions, or what is newer. You can
test it manually, for example with the command:

$ rpmdev-vercmp 0.0.1.beta10-1 0.0.1.beta8-1
0.0.1.beta10-1 > 0.0.1.beta8-1

and what would happen if you called the final version 0.0.1:

$ rpmdev-vercmp 0.0.1-1 0.0.1.beta10-1
0.0.1-1 < 0.0.1.beta10-1

Personally, I prefer to avoid messing the release tag, it is
hard to maintain, and automated scripts cannot properly handle
it.

Michael, I was kind of trying to avoid too much complication
on some packaging details, for the first package :)

Gustavo, do you an ETA of when an official (non beta) package will
be available? It would be desirable that the first official package
in Fedora were not a beta one. But you can follow Michael advice on
the release tag, then, after every package update, increase from
0.1.beta10 to 0.2.beta10 and so on.
Comment 46 Gustavo Lima Chaves 2015-10-28 15:31:21 EDT
$ rpmdev-vercmp 0.0.1-beta10.1 0.0.1-beta8.222
0.0.1-beta10.1 > 0.0.1-beta8.222
$ rpmdev-vercmp 0.0.1-1 0.0.1-beta8.222
0.0.1-1 > 0.0.1-beta8.222

Personally I like the schema above, with just the beta tag + rel number to form the release. Can I go with that?

The ETA for the 1st release is one month or less, we hope. The build issue was fixed locally, BTW.
Comment 47 Paulo Andrade 2015-10-28 16:22:31 EDT
Hi Gustavo,

While most times the warnings about spelling errors are
pointless, there are these:

%description flow-module-form
This package contains the form flow module for %{name}. The modul
eprovides nodes producing formatted, string output to feed LCD

"modul" above is a typo, broke line at wrong position.

...
%description pin-mux-module-galileo
This package contains the galileo pin-mux module for %{name}. The
module provides pin multiplexing rules/mapping for Galileo boards
(revisions D ang G). Without this module, to use Galileo I/O pins with

"ang" above should be "and"?


-----8<-----
The code in sol_memmap_write_raw_do still looks wrong. On little
endian it will only work if reading 8 bytes, or, if initializing
old_value to zero, and reading from 0 to 8 bytes. On big endian
would work only if reading 8 bytes. If entry->size is larger than
8 bytes, it will trash the stack.

The warning about result possibly not initialized in
src/modules/flow/boolean/boolean.c appears sensible, as there is
a code path where it can be used uninitialized, so, it should
be safe to initialize it to zero. But you should know better :)


-----8<-----
About the version, It should be fine as you are upstream
and also submitting the review, and increasing the beta
number every review. Otherwise, it is common practice to
add a "0.1" and increase it, to avoid the case of renaming
the word "beta" in a way that would appear to be older.
As long as you pay attention to rpmdev-vercmp it should
be fine.
You should notice any issues when making a test update
package, *but*, sometimes it may be too late, as the
package was already released, and then need to use the
Epoch rpm tag to fix the mess :)


-----8<-----
I think we are almost done with the review. I would like to have
an extra comment about my question about if you have an ETA for
a stable release. For safety, in case you choose to call the
first official release 0.0.1 it would be better to change back
to Release: 0.1.%{soletta_tag}%{?dist} - but really, pre releases
are avoided as official packages :) Not only for this reason, but
due to not having a somewhat long lived, common release, so that
people talk about the same thing :)
Comment 48 Michael Schwendt 2015-10-28 17:28:34 EDT
It is not my duty to enforce the package versioning guidelines.

I mentioned the guidelines, because %version 0.0.1.beta10 would be problematic, if there were a 0.0.1 final release. Same for a git snapshot made after 0.0.1 and before 0.0.2. Which %version to use then? The guidelines offer some flexibility for such scenarios.

Discussing "the problem", mentioning the existing guidelines and tools like rpmdev-vercmp, may be sufficient.

Still, occasionally packagers run into some of the pitfalls and then can't do much if they don't control upstream's versioning scheme.


> $ rpmdev-vercmp 0.0.1-beta10.1 0.0.1-beta8.222
> 0.0.1-beta10.1 > 0.0.1-beta8.222
> $ rpmdev-vercmp 0.0.1-1 0.0.1-beta8.222
> 0.0.1-1 > 0.0.1-beta8.222

There are many schemes that seem to be compatible with RPM versioning comparison at first glance.

For example, if comparing numbers with letters, numbers win always, so going from release beta8 to 1 seems to work.

It can get awkward, if you publish builds for multiple distribution releases. Suddenly, the "beta8" at the most-significant left side of %release decides whether a package is newer than a build for another dist target. If you ever needed to rebuild a package for an older dist more often than for a newer dist, a different scheme is needed as not to compare build version with %{?dist}.
It can also get troublesome, if you want to switch from a "beta" release to a git snapshot or vice versa. A real release number at the very left opens more opportunities to move pre-release and post-release version details to the less significant right side of %release.

However, some of the problems are not encountered often, and an upstream versioning scheme that is fully compatible with RPM version comparison won't cause any problems.


> and automated scripts cannot properly handle it.

rpmdev-bumpspec as used during mass-rebuilds, handles Fedora's pre-release and post-release versioning schemes just fine. The only requirement is to avoid macro-madness in the Release tag. There are only a very few packages, which would win every spec file obfuscation contest and confuse rpmdev-bumpspec.
Comment 49 Gustavo Lima Chaves 2015-10-29 13:33:51 EDT
> I think we are almost done with the review. I would like to have
> an extra comment about my question about if you have an ETA for
> a stable release. For safety, in case you choose to call the
> first official release 0.0.1 it would be better to change back
> to Release: 0.1.%{soletta_tag}%{?dist} - but really, pre releases
> are avoided as official packages :) Not only for this reason, but
> due to not having a somewhat long lived, common release, so that
> people talk about the same thing :)

Hi, Paulo. For me it's also more reasonable to push the RPM release button
starting from our actual 1st (not pre) release, so let's work with that.
For the next iteration here, that might happen before our upstream release,
disregard the beta tags, which will vanish in sequence. I'll work on
the remaining issues and help with dev here and soon I'll come back
with updates. Thanks a lot, Paulo and Michael!
Comment 50 Paulo Andrade 2015-11-21 15:38:15 EST
Hi Gustavo, since this review has been a bit silent, I did run a full
review again, so, removing some noise of "fedora-review -b 1264546"
on latest rawhide, updated today:

---8<---
[?]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "*No copyright* GPL (v2 or later) (with incorrect FSF
     address)", "GPL (v3 or later)", "Unknown or generated", "PSF (v2)",
     "MIT/X11 (BSD like)", "BSD (3 clause)". 423 files have unknown
     license. Detailed output of licensecheck in
     /home/pcpa/1264546-soletta/licensecheck.txt

  I noticed there are some files with Python license:
soletta-1_beta10/src/modules/flow/converter/LICENSE.PSFL
soletta-1_beta10/src/modules/flow/string/LICENSE.PSFL
  Well, actually, only the python license, the sources there appear to
all use BSD/MIT like license.

  There is also
soletta-1_beta10/tools/kconfig/zconf.tab.c_shipped
with a GPLv3+ license. But I wonder if there any issues regenerating
it, to have the parsed file distributed with sources.

---8<---
[?]: If the package is under multiple licenses, the licensing breakdown
     must be documented in the spec.

  I understand this may easily get outdated on updates, but still
is a good procedure to document it, mostly to show you are aware
of all the source contents.
  Usually, it is comments about sources, e.g.:
# BSD License:
#       some/dir/*
#       some/other/dir/*              (not built/used)
# MIT License:
#       some/dir/*
# GPLv2+
#       some/dir/*
License:        GPLv2+ and BSD and MIT

  And/or also on files section:

%files
# GPLv2+
/file/paths

---8<---
[?]: Package contains no bundled libraries without FPC exception.
  For the moment I believe this is not an issue. Just do not install
headers or .so in -devel packages.

---8<---
[?]: Package functions as described.
  I will trust you, and %check for that :)

---8<---
[?]: Latest version is packaged.
  I believe were waiting for the first release.

---8<---
[?]: Package should compile and build into binary rpms on all supported
     architectures.
  I understand you were working on ensuring the package builds in arm.

---8<---
[?]: Packages should try to preserve timestamps of original installed
     files.
  I kept this as unchecked as there are issues in the make process,
where it rebuilds everything on make install.
Comment 51 Gustavo Lima Chaves 2015-11-30 08:14:23 EST
Thanks a lot, Paulo! I'll be updating the package soon (now for real), with all comments taken into account. We've been rushing with our v1 backlog and I waited a little for the features to get in to package again. This week I'll come back to Fedora packaging. I'll still use tags in the package naming and, again, ignore that for now (that will vanish for the final release).
Comment 52 Upstream Release Monitoring 2015-12-04 12:59:08 EST
glchaves's scratch build of soletta-0.0.1-beta13.1.fc23.src.rpm for rawhide failed http://koji.fedoraproject.org/koji/taskinfo?taskID=12055290
Comment 53 Upstream Release Monitoring 2015-12-04 13:50:41 EST
glchaves's scratch build of soletta-0.0.1-beta13.1.fc23.src.rpm for rawhide completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12055502
Comment 54 Gustavo Lima Chaves 2015-12-04 14:03:10 EST
New spec URL: https://raw.githubusercontent.com/solettaproject/soletta-packaging/v9/rpm/soletta.spec

New SRPM URL: https://github.com/solettaproject/soletta-packaging/releases/download/v9/soletta-0.0.1-beta13.1.fc23.src.rpm

koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=12055502

Paulo, the warning on the coap code is already fixed here and will vanish in the next iteration. The warning on that memmap code is still there, but as we guarantee it won't happen, we have an assert to check that the size of the destination buffer is enough.

All other issues were fixed, I hope.

We'll take some time to reach 1.0 (think beginning of 2016). The version+release tag is in a way that at least conforms to rpmdev-vercmp. So it's up to you guys if we can proceed with a beta package or wait to the release.

Thanks a lot for the support :)
Comment 55 Gustavo Lima Chaves 2016-01-17 21:05:30 EST
Ping. Have you had the time to take a look, Paulo? I can bump the spec to newer code before that if you prefer too, no big deal. We're closer to the 1st release now, and it would be nice to have all packaging issues gone ASAP :)

Thanks a lot.
Comment 56 Paulo Andrade 2016-01-18 07:02:36 EST
(In reply to Gustavo Lima Chaves from comment #55)

  Hi Gustavo,

> Ping. Have you had the time to take a look, Paulo? I can bump the spec to
> newer code before that if you prefer too, no big deal. We're closer to the
> 1st release now, and it would be nice to have all packaging issues gone ASAP
> :)

  :)

  Nice to know it is getting close to a final release.
  I will make an extra informal review of the 12/04 package now,
and should give feedback shortly.

> Thanks a lot.
Comment 57 Paulo Andrade 2016-01-18 12:39:44 EST
  Hi Gustavo.

  The package looks pretty good now.

  There is still the issue of it building twice, but since the
package builds fast, and you know already know about it, it is
not a blocker.

  I am only in doubt about usage of PSF for license, so I asked
@packaging about it, as it might need to use PSFL, or follow the
pattern of other packages, that use simply "Python" when referring
to PSF(L). I will let you know about any news I have about it.
Comment 58 Gustavo Lima Chaves 2016-01-20 07:29:05 EST
Thanks, Paulo! I'll wait for the verdict on the Python Licensing citation, then. Sad that the build issue is still there, we'll put more effort to fix that for the next iteration (soon I'll have one posted). Cheers.
Comment 59 Gustavo Lima Chaves 2016-05-02 09:44:01 EDT
The next iteration is coming soon. We're really rushing towards 1.0 here, and since we had agreed on only packaging with a release (out of beta state), I'm focusing more on getting to it than updating beta specs, OK? But we're almost there now, just as a heads up.
Comment 60 Gustavo Lima Chaves 2016-08-04 16:16:46 EDT
https://github.com/solettaproject/soletta-packaging/pull/26 awaiting review, now
Comment 61 Gustavo Lima Chaves 2016-08-26 09:59:21 EDT
v1.0 ack-ed on our side, with rpmlint happy for all assets. Now going for Fedora's infra back again (koji build).
Comment 62 Gustavo Lima Chaves 2016-08-26 13:35:54 EDT
New spec URL: https://raw.githubusercontent.com/solettaproject/soletta-packaging/v11/rpm/soletta.spec

New SRPM URL: https://github.com/solettaproject/soletta-packaging/releases/download/v11/soletta-1-1.fc24.src.rpm

koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=15389537

Hi, Paulo. Finally, we have come to this -- v1 both on our side and on the RPM spec. Sorry for taking longer than needed, but a lot of development took place in order for this to happen. We're now a stable API, at least, and hope to benefit many Fedora users with the ease of packaging.

There are close to 0 rpmlint complaints, I hope we're good now. The libmicrohttpd issue has been solved by pushing its usage to future releases, when the lib version in the system has been bumped.

Let me know if there are any issues left.
Comment 63 Paulo Andrade 2016-09-01 12:19:44 EDT
1. Please add an owner to the directory /usr/share/soletta/flow/aliases

2. I suggest creating a ChangeLog file, and installing it
under /usr/share/doc/soletta-devel. The rpm spec %changelog
is not development changelog :)
See https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs
specifically:
"""
The intent is to give the user a hint as to what changed in a package update without overwhelming them with the technical details.
"""
It is not a blocking issue, just that it is not the right place
for development changelog.
Most times a changelog file can be created on the fly with
"git shortlog", but you may prefer to have a more well
formatted one.

  Please consider the comments above for a minor update before
the final build.

  I consider the package approved.
Comment 64 Paulo Andrade 2016-09-01 12:23:32 EDT
Gustavo,

Welcome as a Fedora packager!

Please see remaining information at
https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner

I will be your sponsor. If you have any issues,
please send me an email, or for more urgent issues
you can also ping me in freenode, nickname pcpa.

I will do some extra duties as sponsor, like watching
you bugzilla interactions, but that is just for the
bureaucracy part :)
Comment 65 Gwyn Ciesla 2016-09-01 12:30:33 EDT
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/soletta
Comment 66 Gustavo Lima Chaves 2016-09-01 12:41:50 EDT
Thanks a lot, Paulo!

I'll do the alias file reference, sorry for that (rpmlint failed me on that, I think :/).

As for Changelogs, can I simply edit the ones I got, removing non-RPM packaging relevant stuff (with no new changelog entries on the spec)?
Comment 67 Paulo Andrade 2016-09-03 12:05:44 EDT
  Hi Gustavo,

  About the changelog, feel free to to how you think is
better.
  For now there isn't much logs in the changelog, but if
you kept that way, soon you could have a really large
changelog :)
Comment 68 Gustavo Lima Chaves 2016-09-05 12:34:12 EDT
New spec URL: https://raw.githubusercontent.com/solettaproject/soletta-packaging/v12/rpm/soletta.spec

New SRPM URL: https://github.com/solettaproject/soletta-packaging/releases/download/v12/soletta-1-1.fc24.src.rpm

koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=15507592

Please let me know if we still got issues or if further instructions on how to proceed are yet unknown to me.

Thanks a lot.
Comment 69 Fedora Update System 2016-09-09 10:30:26 EDT
soletta-1-1.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2016-a45444cb7d
Comment 70 Fedora Update System 2016-09-09 10:53:28 EDT
soletta-1-1.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-8f80075e7f
Comment 71 Fedora Update System 2016-09-10 02:52:18 EDT
soletta-1-1.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-8f80075e7f
Comment 72 Fedora Update System 2016-09-10 03:57:24 EDT
soletta-1-1.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-a45444cb7d
Comment 73 Fedora Update System 2016-09-13 14:08:06 EDT
soletta-1-1.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.
Comment 74 Gustavo Lima Chaves 2016-09-13 14:15:44 EDT
Well, I'm still waiting for the Fedora 24 testing request. Also, on rawhide, we're getting hit with errors on builds for the new aarch64 target -- a floating point issue detected by our tests. We'll have to attack that on a subsequent release.
Comment 75 Parag AN(पराग) 2016-09-15 12:01:55 EDT
This closed review should not keep blocking FE-NEEDSPONSOR.
Comment 76 Fedora Update System 2016-09-21 20:25:04 EDT
soletta-1-1.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.

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