Skip to content

Conversation

@Olf0
Copy link
Contributor

@Olf0 Olf0 commented Feb 27, 2022

  • Introduce %defattr(-,root,root,-) in %files section
  • Alter package name: test-casestestcases, because it becomes prefixed with %{name}- to patchmanager-testcases (avoiding two hyphens).
  • Obey SailfishOS:Chum PR 70
  • Overhaul descriptions: Summary:, %description and "Chum metadata"
  • Generically applied %{url} at many places and %{name} at a few additional ones.
  • Clean up %files testcase section conforming to classic, authoritative references.

- Introduce `%defattr(-,root,root,-)` in `%files` section
- Prefix / alter package name: `test-cases`→`patchmanager-testcases`
- Obey [SailfishOS:Chum PR 70](sailfishos-chum/main#70)
- Overhaul descriptions: `Summary:` and `%description`
- Generically applied `%{url}` at many places and `%{name}` at a few additional ones.
@Olf0 Olf0 requested a review from nephros February 27, 2022 02:17
@Olf0
Copy link
Contributor Author

Olf0 commented Feb 27, 2022

@nephros, too late I became aware, that I did not really review the test cases branch.
I did that for the spec file, now. Please check:

@Olf0
Copy link
Contributor Author

Olf0 commented Feb 27, 2022

Checks & Balances question: Does the rpmlintrc file need some adaption(s) to also properly check these new parts of the spec file?


%package test-cases
Summary: test cases for %{name}
%package %{name}-testcases
Copy link
Contributor

@nephros nephros Feb 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn´t do what we want.

A subpackage in a .spec is automatically prefixed with the main package name.

This change would result in a patchmanager-patchmanager-testcases rpm.

Either use

%package -n %{name}-testcases

which would work but is redundant

or

%package testcases

if you want to drop the hyphen

PS: example build here: https://build.sailfishos.org/package/binaries/home:nephros:devel/patchmanager?repository=sailfishos_4.3_arm

Copy link
Contributor Author

@Olf0 Olf0 Feb 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for reviewing: While I have ("involuntary" due to packaging my little scripts for SFOS) some extended experience with RPM, I never used sub-packages before. Hence I was not aware of this detail, the automatic prefixing with %{name}-.

So I reverted this (three times), but left out what ultimately did become the "second hyphen", see commit 6e5a006.

systemctl-user daemon-reload

%files test-cases
%files %{name}-testcases
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, see comment on %package above.

Copy link
Contributor Author

@Olf0 Olf0 Feb 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

… and a third time for the sub-package's %description (commit 6e5a006).

Copy link
Contributor

@nephros nephros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure all the macros using %{url} and friends don't trip up rpmlint somewhere.

Have you tested that those do what they are intended to do? [EDIT:] Nope, rpmlint at least is fune with it according to https://build.sailfishos.org/package/binaries/home:nephros:devel/patchmanager test build.

Also see inline coments about package name.

@nephros
Copy link
Contributor

nephros commented Feb 27, 2022

Please check:

  • Introduce %defattr(-,root,root,-) in %files section

    IMO it was missing. I assume rpmlint would have complained.
    Is the usual -,root,root,- correct here?

Yes it is and should be there.
Without it, the permissions set in the %install phase are used, and some build environments (including the one on chum) set 755 or something like that on all source files per default so many files run the risk of being installed executable.

@Olf0
Copy link
Contributor Author

Olf0 commented Feb 28, 2022

I took a closer look at the %files section and cleaned it up by commit a41e9a9:

  1. IMO any <directory>/* (…works, but…) should be just <directory>.
  2. The sequence
    %dir <directory>
    <directory>
    does not make any sense, IMO.

Reference, e.g., http://ftp.rpm.org/max-rpm/s1-rpm-inside-files-list-directives.html#S3-RPM-INSIDE-DIR-DIRECTIVE

Please re-review!

P.S.:

Have you tested that those do what they are intended to do?

No, as so often due to being "outta time", unfortunately.
I should improve on that, especially because …

[EDIT:] Nope, rpmlint at least is fine with it according to https://build.sailfishos.org/package/binaries/home:nephros:devel/patchmanager test build.

One does not need to perform separate test builds: On every commit to a PR destined for the master branch a test build is automatically performed at Patchmanager's GitHub-actions build-runner. A freshly built RPM is usually available a few minutes after each commit.

BTW, the %{name}, %{url}, %{summary}, %{version} etc. "variable" evaluations are substituted very early when rpmbuild processes a spec file: I have never seen these substitutions fail or happening too late (AFAIR this is impossible).

Also see inline comments about package name.

They also nicely displayed here (above) at the Conversations "tab" of a PR in GitHub's web-frontend, not only as inline comments at the Files changed "tab": Thanks, resolved.

@Olf0 Olf0 self-assigned this Feb 28, 2022
@Olf0 Olf0 added this to the 3.2.2 milestone Feb 28, 2022
@Olf0 Olf0 added bug something that needs fixing documentation documentation, Wiki and related enhancement this improves something infrastructure building, packaging, hosting etc. labels Feb 28, 2022
@Olf0 Olf0 requested a review from nephros February 28, 2022 02:48
@nephros
Copy link
Contributor

nephros commented Feb 28, 2022

One does not need to perform separate test builds: On every commit to a PR destined for the master branch a test build is automatically performed at Patchmanager's GitHub-actions build-runner. A freshly built RPM is usually available a few minutes after each commit.

Yes, but it builds the exactly branch the PR originates from, while my "obs" branch is master plus whatever changes I want to test against (usually the pr branch plus maybe some quick fixes if, or another branch I want to test together with the PR).

@nephros
Copy link
Contributor

nephros commented Feb 28, 2022

I took a closer look at the %files section and cleaned it up by commit a41e9a9:

  1. IMO any <directory>/* (…works, but…) should be just <directory>.
  2. The sequence
    %dir <directory>
    <directory>
    does not make any sense, IMO.

Thanks that's better indeed.

@Olf0 Olf0 merged commit 591bd17 into master Feb 28, 2022
@Olf0 Olf0 deleted the Olf0-patch-1 branch February 28, 2022 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug something that needs fixing documentation documentation, Wiki and related enhancement this improves something infrastructure building, packaging, hosting etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants