[Bug 1471056] Review Request: postsrsd - Sender Rewriting Scheme (SRS) daemon

[bugzilla]

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

Marc Dequènes (Duck)  changed:

   What|Removed |Added

 Status|POST|CLOSED
 Resolution|--- |RAWHIDE
Last Closed||2017-10-04 13:59:27



--- Comment #21 from Marc Dequènes (Duck)  ---
Thanks Robert-André

-- 
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 1471056] Review Request: postsrsd - Sender Rewriting Scheme (SRS) daemon

[bugzilla]

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



--- Comment #20 from Robert-André Mauchin  ---
It's not automated, you should close it yourself and select Rawhide as a
resolution.

-- 
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 1471056] Review Request: postsrsd - Sender Rewriting Scheme (SRS) daemon

[bugzilla]

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



--- Comment #19 from Marc Dequènes (Duck)  ---
Quack,

I had already made a build. I fixed it in postsrsd-1.4-9.20170118gita77bf9.
Thanks for your nice suggestions.

Now I wonder what I might have missed because I guess this ticket should be
closed now, and my other one too. Is it not automagically closed when a
successful build is done?

-- 
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 1471056] Review Request: postsrsd - Sender Rewriting Scheme (SRS) daemon

[bugzilla]

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

Andreas Thienemann  changed:

   What|Removed |Added

 CC||andr...@bawue.net



--- Comment #18 from Andreas Thienemann  ---
Thank you for submitting the package and doing all the work to get it into
Fedora.

I did notice one thing about the .spec file though I did not like. The handling
of the postsrsd.secret file could be improved.

I like that you generate it in the %post script, thus ensuring that every
install has a unique file.
But I do not like the way you clean up the file in the %postun scriptlet.

The way it is handled right now is that there's a file in /etc/ that does not
belong to any package.

# rpm -ql postsrsd | grep -F postsrsd.secret
# rpm -qf /etc/postsrsd.secret
file /etc/postsrsd.secret is not owned by any package
#

I would expect the postsrsd package to own this file.
You can achieve that by adding the filepath with the %ghost tag to the spec
file in the %files section.
http://ftp.rpm.org/max-rpm/s1-rpm-inside-files-list-directives.html has some
details.

The rm -f command can be removed then.
Please fix that before uploading the 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 1471056] Review Request: postsrsd - Sender Rewriting Scheme (SRS) daemon

[bugzilla]

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



--- Comment #17 from Gwyn Ciesla  ---
(fedrepo-req-admin):  The Pagure repository was created at
https://src.fedoraproject.org/rpms/postsrsd

-- 
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 1471056] Review Request: postsrsd - Sender Rewriting Scheme (SRS) daemon

[bugzilla]

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



--- Comment #16 from Marc Dequènes (Duck)  ---
Taking care of the latest fixes.

Thanks a lot :-).

-- 
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 1471056] Review Request: postsrsd - Sender Rewriting Scheme (SRS) daemon

[bugzilla]

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

Matthias Runge  changed:

   What|Removed |Added

 Blocks|177841 (FE-NEEDSPONSOR) |



--- Comment #15 from Matthias Runge  ---
Two more nits: please break the description line < 79 chars. Also: the build
requirement on gcc is not necessary.

Since I've already sponsored you into the packager group, you can go ahead and
submit a request for a git repo to be created. Feel free to ping me, if there
are any questions.


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 1471056] Review Request: postsrsd - Sender Rewriting Scheme (SRS) daemon

[bugzilla]

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



--- Comment #14 from Marc Dequènes (Duck)  ---
SPEC:
https://gitlab.com/osas/osas-infra-team-rpm-pkg/raw/postsrsd-1.4-7.20170118gita77bf99/postsrsd/postsrsd.spec
SRPM URL:
https://copr-be.cloud.fedoraproject.org/results/duck/osas-infra-team-rpm-repo/epel-7-x86_64/00608856-postsrsd/postsrsd-1.4-7.20170118gita77bf99.el7.centos.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 1471056] Review Request: postsrsd - Sender Rewriting Scheme (SRS) daemon

[bugzilla]

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



--- Comment #13 from Marc Dequènes (Duck)  ---
-7 has the needed changes.

-- 
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 1471056] Review Request: postsrsd - Sender Rewriting Scheme (SRS) daemon

[bugzilla]

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

Matthias Runge  changed:

   What|Removed |Added

 CC||mru...@redhat.com



--- Comment #12 from Matthias Runge  ---
A few more remarks:
- please separate changelog entries by empty lines:

* Tue Aug 22 2017 Marc Dequènes (Duck)  -
1.4-5.20170118gita77bf99
- remove %%clean section, not needed in Fedora

* Tue Aug 22 2017 Marc Dequènes (Duck)  -
1.4-4.20170118gita77bf99
- don't remove secret file during upgrade
- start service at the end of post scriptlet
- improve SELinux rules handling (now requires a running SELinux)

* Tue Aug 22 2017 Marc Dequènes (Duck)  -
1.4-3.20170118gita77bf99


you shouldn't remove buildroot and create it again. That can be expected to be
clean 
(begin of %install section)

On irc, you mentioned another package waiting for review. Please point me to
that, I might be able to get you sponsor you.

-- 
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 1471056] Review Request: postsrsd - Sender Rewriting Scheme (SRS) daemon

[bugzilla]

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



--- Comment #11 from Marc Dequènes (Duck)  ---
Yeahh :-)
Thank you for your help.

-- 
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 1471056] Review Request: postsrsd - Sender Rewriting Scheme (SRS) daemon

[bugzilla]

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

Robert-André Mauchin  changed:

   What|Removed |Added

 Status|NEW |POST
   Assignee|nob...@fedoraproject.org|zebo...@gmail.com
  Flags||fedora-review+



--- Comment #10 from Robert-André Mauchin  ---
We're all good then, package accepted.


Thanks for your 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 1471056] Review Request: postsrsd - Sender Rewriting Scheme (SRS) daemon

[bugzilla]

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



--- Comment #9 from Marc Dequènes (Duck)  ---
That's right, the buildsys already takes care of installing the doc.

I made the changes.

-- 
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 1471056] Review Request: postsrsd - Sender Rewriting Scheme (SRS) daemon

[bugzilla]

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



--- Comment #8 from Robert-André Mauchin  ---
I forgot a few thing:

 - make is not needed as a BR
 - /etc/default/ → %{_sysconfdir}/default in %files
 - Contrary to what I've told you before, remove:

%doc README.md README_UPGRADE.md README.exim.md

 It creates conflicts and duplicate files with the %{_docdir} macro, and usage
of both in the same spec is disallowed.

-- 
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 1471056] Review Request: postsrsd - Sender Rewriting Scheme (SRS) daemon

[bugzilla]

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



--- Comment #7 from Marc Dequènes (Duck)  ---
Done Sir :-).

-- 
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 1471056] Review Request: postsrsd - Sender Rewriting Scheme (SRS) daemon

[bugzilla]

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



--- Comment #6 from Robert-André Mauchin  ---
You also need to remove the %clean section which is not needed in Fedora:
https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections

-- 
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 1471056] Review Request: postsrsd - Sender Rewriting Scheme (SRS) daemon

[bugzilla]

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



--- Comment #5 from Marc Dequènes (Duck)  ---
It leaves a few questions unanswered but I went on.

Also I dug some more around the selinux example and looked more in details at
the scriptlets scheduling. It's very peculiar to see upgrade actions launched
at the postun stage (supposedly "uninstall" stuff) because at this point the
old package possible leftover files are no more and it is safe to restart the
service and so on…

Anyway, considering previous comments and adaptations due to the fact we don't
need 'service' as we're relying on systemd, I made changes to the package.

I'm currently running this version in production and it seem to work fine.

Would you please have another look?

-- 
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 1471056] Review Request: postsrsd - Sender Rewriting Scheme (SRS) daemon

[bugzilla]

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



--- Comment #4 from Robert-André Mauchin  ---
>As for the version, the following fix was needed to make the service usable at 
>all

All right then.


>As for SELinux, could you tell me where these information come from, it is not 
>in the Fedora Packaging guidelines?

The SELinux packaging draft. See
https://fedoraproject.org/wiki/PackagingDrafts/SELinux at the bottom.

> So the fishy part was about allowing the scritlets to work on machines 
> without SELinux activated

SELinux is enabled by default on Fedora. As you see above in the TODO, the case
where SELinux is disabled is not considered.

-- 
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 1471056] Review Request: postsrsd - Sender Rewriting Scheme (SRS) daemon

[bugzilla]

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



--- Comment #3 from Marc Dequènes (Duck)  ---
Thanks Robert-André for the review.

As for the version, the following fix was needed to make the service usable at
all: https://github.com/roehling/postsrsd/pull/65
I should have made it clearer in the changelog and will fix this.

You're right, base64 is part of coreutils. The following page tells me a
BuildRequires on it is unnecessary:
https://fedoraproject.org/wiki/HOWTOFindMissingBuildRequires
It is ok for this package, nevertheless this is really not satisfying. How are
we supposed to know the list of these packages. Someone on the Internet
suggested it could be packages in group 'Core' but got no interesting reply. I
can't find more documentation on this, could point at the canonical source of
info?


As for SELinux, could you tell me where these information come from, it is not
in the Fedora Packaging guidelines?

So the fishy part was about allowing the scritlets to work on machines without
SELinux activated. Is Fedora officially not supporting such configurations?

Are these scriplets requires really necessary, it looks to me packages from a
base installation, and also in the 'Core' group?

I did not know fixfiles, really practical; I would still need to call
restorecon manually for /etc/postsrsd.secret as it is generated and not in
%files. Or is there another way?

Also calling semodule only at first install seems not a good idea. The daemon
might change and require new rules, so it's safer to rebuild after each upgrade
to avoid breaking the service. Same for fixfiles. And then same for restarting
the service. Am I wrong?

Also what is "|| :"? Seems to behave like "|| true" but I don't find it very
readable (might depend on your font size though).

So I'm going to push fixes which are well understood. The rest would come soon.
Regards.
\_o<

-- 
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 1471056] Review Request: postsrsd - Sender Rewriting Scheme (SRS) daemon

[bugzilla]

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

Robert-André Mauchin  changed:

   What|Removed |Added

 CC||zebo...@gmail.com



--- Comment #2 from Robert-André Mauchin  ---
Hello/Quack,

Several issues:

 - Please don't use tabs, use spaces

 - What do you package a snapshot of the git, instead of the latest stable
version? Please justify this.

 - If you do want to package the snapshot, the Version and Release tag are
wrong. First you should define a %commitdate

%global commit date 20170118

  Then fix you Version and Release:

Version:1.4
Release:2.%{commitdate}git%{shortcommit0}%{?dist}

  Don't forget to also change the changelog to reflect this:

* Fri Apr 14 2017 Marc Dequènes (Duck)  -
1.4-2.20170118gita77bf99

 - The Group: tag is not used in Fedora. See
https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections
If you also package for RHEL, put it inside a condition.


 - The Source0 format is wrong, change it to:

Source0:   
https://github.com/roehling/%{name}/archive/%{commit0}/%{name}-%{commit0}.tar.gz

  - Requires:/usr/bin/base64 is not needed I think, since it is provided by
coreutils

  - Don't use:

cmake .. -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_FLAGS="$(RPM_OPT_FLAGS)"
-DCMAKE_INSTALL_PREFIX=/usr -DGENERATE_SRS_SECRET=OFF -DUSE_SELINUX=ON

  Instead please use the %cmake macro which takes care of the path and add your
custom option afterwards:

cd build && %cmake .. -DGENERATE_SRS_SECRET=OFF -DUSE_SELINUX=ON

  - Similarly, please use %make_build and %make_install macro:

%make_build -C build

%make_install -C build

  - # proper location for systemd config ⇒ This is not the propre location,
service file must be placed in the special dir %{_unitdir}:

mkdir -p %{buildroot}/%{_unitdir}
mv %{buildroot}/etc/systemd/system/postsrsd.service
%{buildroot}/%{_unitdir}/postsrsd.service


  You also need to change the path in your sed patch:

sed -ri -e 's/postsrsd\/default/postsrsd.default/' \
-e
"s/(\[Install\])/RuntimeDirectory=postsrsd\nRuntimeDirectoryMode=0750\n\n\1/"
%{buildroot}/%{_unitdir}/postsrsd.service


  And fix it in %files too:

%{_unitdir}/postsrsd.service

  - systemd service files requires special scriplets to be run in %post, %preun
and %postun. See:
https://fedoraproject.org/wiki/Packaging:Scriptlets?rd=Packaging:ScriptletSnippets#Systemd


%post
%systemd_post %{name}.service

%preun
%systemd_preun %{name}.service

%postun
%systemd_postun_with_restart %{name}.service

  You also need to add a BR with a special macro:

%{?systemd_requires}
BuildRequires: systemd

  - You *must* add the LICENSE file with %license in %files:

%license LICENSE

  - %doc should contain the relevant README files:

%doc README.md README_UPGRADE.md README.exim.md

  - Don't use /usr/sbin, use %{_sbindir}

%{_sbindir}/postsrsd


  - Don't use /usr/share/doc but:

%{_docdir}/postsrsd

  - Same with man:

%{_mandir}/man8/postsrsd.8*

  - The whole SELinux part looks fishy. Let's do it the Fedora way. First add
the Requires:

Requires(post): policycoreutils, initscripts
Requires(preun): policycoreutils, initscripts
Requires(postun): policycoreutils

  Then we add the correct scriplets in %post, %preun and %postun:

%post
if [ "$1" -le "1" ]; then # Fist install
semodule -i %{_datadir}/selinux/packages/%{name}/postsrsd.pp 2>/dev/null ||
:
fixfiles -R postsrsd restore || :
/sbin/service postsrsd condrestart > /dev/null 2>&1  || :
fi

%preun
if [ "$1" -lt "1" ]; then # Final removal
semodule -r postsrsd 2>/dev/null || :
fixfiles -R postsrsd restore || :
/sbin/service postsrsd condrestart > /dev/null 2>&1 || :
fi

%postun
if [ "$1" -ge "1" ]; then # Upgrade
# Replaces the module if it is already loaded
semodule -i %{_datadir}/selinux/packages/%{name}/postsrsd.pp 2>/dev/null ||
:
# no need to restart the daemon
fi


  Once you fix all this, please check if the package is working as expected as
I'm not well versed in SELinux.

-- 
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 1471056] Review Request: postsrsd - Sender Rewriting Scheme (SRS) daemon

[bugzilla]

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

Marc Dequènes (Duck)  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 1471056] Review Request: postsrsd - Sender Rewriting Scheme (SRS) daemon

[bugzilla]

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



--- Comment #1 from Marc Dequènes (Duck)  ---
Quack,

As for the build problems, it was partially solved in ticket #1471066 and is
being continued in #1473361 and #1473364. The build was relaunched and works
well on all F25/F26/EL7 distros and related architectures.

Could someone haz a look pleaze?
\_o<

-- 
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