Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Address feedback from Fedora review #715

Closed
17 tasks done
jw3 opened this issue Dec 20, 2022 · 9 comments
Closed
17 tasks done

Address feedback from Fedora review #715

jw3 opened this issue Dec 20, 2022 · 9 comments
Labels
build feedback1 rpm
Milestone
v1.0

Comments

@jw3
Copy link
Member

jw3 commented Dec 20, 2022 •
edited
Loading

The bz issue is #2153687

The Copr build page is here

The initial review shows several [!] findings that need addressed. Going to track them all together here.

Update: Repository

Review 1

  • [?]: Package contains desktop file if it is a GUI application.
  • [!]: Sources are verified with gpgverify first in %prep if upstream publishes signatures.
  • [!]: Package meets the Packaging Guidelines::Python -- Please, use macros instead of python3 interpreter.
  • Suggested: use "tar -xzf" instead of "tar xzf"
  • Suggested: use "%autosetup -p0" instead of "%autosetup -p1"
  • Rpmlint: E: explicit-lib-dependency dbus-libs
  • Rpmlint: W: no-manual-page-for-binary fapolicy-analyzer
  • Rpmlint: W: invalid-url Source1: vendor-rs.tar.gz
  • Rpmlint: W: invalid-url Source0: fapolicy-analyzer.tar.gz
  • Rpmlint: W: incoherent-version-in-changelog 0.6.1-1 ['0.6.2-1.fc38', '0.6.2-1']
  • Rpmlint: W: files-duplicate /usr/lib64/python3.11/site-packages/fapolicy_analyzer/util/init.py /usr/lib64/python3.11/site-packages/fapolicy_analyzer/css/init.py:/usr/lib64/python3.11/site-packages/fapolicy_analyzer/glade/init.py:/usr/lib64/python3.11/site-packages/fapolicy_analyzer/resources/init.py
  • Rpmlint: W: files-duplicate /usr/lib64/python3.11/site-packages/fapolicy_analyzer/resources/sourceview/styles/init.py /usr/lib64/python3.11/site-packages/fapolicy_analyzer/resources/sourceview/language-specs/init.py
  • Rpmlint: W: file-not-in-%lang /usr/lib64/python3.11/site-packages/fapolicy_analyzer/locale/es/LC_MESSAGES/fapolicy_analyzer.mo

Review 2

  • fapolicy-analyzer.x86_64: W: one-line-command-in-%post update-desktop-database
  • fapolicy-analyzer.x86_64: W: no-manual-page-for-binary fapolicy-analyzer

(only new and reopened findings are included)

Review 3

  • [!]: Spec file according to URL is the same as in SRPM.

More todo

  • Add desktop file validate to the %check section
    • desktop-file-validate %{buildroot}/%{_datadir}/applications/%{name}.desktop
@jw3 jw3 added this to the v1.0 milestone Dec 20, 2022
@jw3 jw3 mentioned this issue Dec 20, 2022
Closed
@jw3
Copy link
Member Author

jw3 commented Dec 20, 2022

Rpmlint findings

fapolicy-analyzer.x86_64: W: no-manual-page-for-binary fapolicy-analyzer
fapolicy-analyzer.spec: W: invalid-url Source1: vendor-rs.tar.gz
fapolicy-analyzer.spec: W: invalid-url Source0: fapolicy-analyzer.tar.gz
fapolicy-analyzer.x86_64: W: incoherent-version-in-changelog 0.6.1-1 ['0.6.2-1.fc38', '0.6.2-1']
fapolicy-analyzer.x86_64: W: files-duplicate /usr/lib64/python3.11/site-packages/fapolicy_analyzer/util/__init__.py /usr/lib64/python3.11/site-packages/fapolicy_analyzer/css/__init__.py:/usr/lib64/python3.11/site-packages/fapolicy_analyzer/glade/__init__.py:/usr/lib64/python3.11/site-packages/fapolicy_analyzer/resources/__init__.py
fapolicy-analyzer.x86_64: W: files-duplicate /usr/lib64/python3.11/site-packages/fapolicy_analyzer/resources/sourceview/styles/__init__.py /usr/lib64/python3.11/site-packages/fapolicy_analyzer/resources/sourceview/language-specs/__init__.py
fapolicy-analyzer.x86_64: W: file-not-in-%lang /usr/lib64/python3.11/site-packages/fapolicy_analyzer/locale/es/LC_MESSAGES/fapolicy_analyzer.mo
fapolicy-analyzer.x86_64: E: explicit-lib-dependency dbus-libs
============================================================================================ 4 packages and 0 specfiles checked; 1 errors, 7 warnings, 1 badness; has taken 1.0 s

@jw3
Copy link
Member Author

jw3 commented Dec 21, 2022

Suggested: use "%autosetup -p0" instead of "%autosetup -p1"

Should ask for clarity when resubmitting, because this was already in the requested state.

@jw3
Copy link
Member Author

jw3 commented Dec 21, 2022

Rpmlint: W: incoherent-version-in-changelog 0.6.1-1

This was an oversight when bumping the version.

This was referenced Dec 23, 2022
Merged
Closed
@jw3
Copy link
Member Author

jw3 commented Dec 29, 2022

This is blocked by #722

@jw3
Copy link
Member Author

jw3 commented Dec 29, 2022

[!]: Sources are verified with gpgverify first in %prep if upstream publishes signatures.

We have no source to verify from

jw3 added a commit that referenced this issue Dec 29, 2022
@jw3
RPM build updates (#718)
ecd1eef 
Addresses findings from the first Fedora review.

Also includes some other distantly related, but supportive, changes.

Addresses several findings from first Fedora review (#715):
- Adding desktop file
- Adding man page
- Proper installation of translated help
- Cleaning up lints in the spec 
- Reduces rpm deps by removing tool-only deps

Also
- Upgrades Clap
- Makes the top level Rust project a virtual project
- Makes crates/tools a standalone crate
- Upgrades some Actions
- Adds a checksum to wf to aid in debugging

Closes #616  
Closes #722
Closes #723
jw3 added a commit that referenced this issue Dec 30, 2022
@jw3
RPM build updates (#718)
627607e 
Addresses findings from the first Fedora review.

Also includes some other distantly related, but supportive, changes.

Addresses several findings from first Fedora review (#715):
- Adding desktop file
- Adding man page
- Proper installation of translated help
- Cleaning up lints in the spec
- Reduces rpm deps by removing tool-only deps

Also
- Upgrades Clap
- Makes the top level Rust project a virtual project
- Makes crates/tools a standalone crate
- Upgrades some Actions
- Adds a checksum to wf to aid in debugging

Closes #616
Closes #722
Closes #723
# Conflicts:
#	Cargo.toml
#	crates/pyo3/src/check.rs
#	crates/tools/src/fapolicy_profiler.rs
#	fapolicy-analyzer.spec
#	help/__init__.py
#	scripts/srpm/fapolicy-analyzer.spec
jw3 added a commit that referenced this issue Dec 30, 2022
@jw3
RPM build updates (#718)
e1ca7d4 
Addresses findings from the first Fedora review.

Also includes some other distantly related, but supportive, changes.

Addresses several findings from first Fedora review (#715):
- Adding desktop file
- Adding man page
- Proper installation of translated help
- Cleaning up lints in the spec
- Reduces rpm deps by removing tool-only deps

Also
- Upgrades Clap
- Makes the top level Rust project a virtual project
- Makes crates/tools a standalone crate
- Upgrades some Actions
- Adds a checksum to wf to aid in debugging

Closes #616
Closes #722
Closes #723
# Conflicts:
#	Cargo.toml
#	crates/pyo3/src/check.rs
#	crates/tools/src/fapolicy_profiler.rs
#	fapolicy-analyzer.spec
#	help/__init__.py
#	scripts/srpm/fapolicy-analyzer.spec
jw3 added a commit that referenced this issue Jan 2, 2023
@jw3
RPM build updates (#718)
d29ad28 
Addresses findings from the first Fedora review.

Also includes some other distantly related, but supportive, changes.

Addresses several findings from first Fedora review (#715):
- Adding desktop file
- Adding man page
- Proper installation of translated help
- Cleaning up lints in the spec
- Reduces rpm deps by removing tool-only deps

Also
- Upgrades Clap
- Makes the top level Rust project a virtual project
- Makes crates/tools a standalone crate
- Upgrades some Actions
- Adds a checksum to wf to aid in debugging

Closes #616
Closes #722
Closes #723
# Conflicts:
#	Cargo.toml
#	crates/pyo3/src/check.rs
#	crates/tools/src/fapolicy_profiler.rs
#	fapolicy-analyzer.spec
#	help/__init__.py
#	scripts/srpm/fapolicy-analyzer.spec
@jw3 jw3 mentioned this issue Jan 6, 2023
Open
@jw3 jw3 pinned this issue Jan 8, 2023
This was referenced Jan 9, 2023
Closed
Merged
jw3 added a commit that referenced this issue Jan 9, 2023
@jw3
Eliminate vendoring on Rawhide (#730)
e585d92 
It has become possible to eliminate crate vendoring all together for
Rawhide.

This PR removes vendoring from the top level spec, Containerfile, and CI.

This also:
- Drops fc37 build
- Bumps nom to v7.1
- Switches from lmdb-rkv to lmdb v0.8
- Bumps other deps for Fedora package compat
- Uses urls for SourceX rather than relative paths

#715
jw3 added a commit that referenced this issue Jan 9, 2023
@jw3
Eliminate vendoring on Rawhide (#730)
6477204 
It has become possible to eliminate crate vendoring all together for
Rawhide.

This PR removes vendoring from the top level spec, Containerfile, and CI.

This also:
- Drops fc37 build
- Bumps nom to v7.1
- Switches from lmdb-rkv to lmdb v0.8
- Bumps other deps for Fedora package compat
- Uses urls for SourceX rather than relative paths

#715
# Conflicts:
#	.copr/Makefile
#	.github/workflows/rpm.yml
#	Cargo.lock
#	Containerfile
#	crates/trust/Cargo.toml
#	crates/trust/src/load.rs
#	crates/util/Cargo.toml
#	fapolicy-analyzer.spec
#	scripts/srpm/fapolicy-analyzer.spec
jw3 added a commit that referenced this issue Jan 9, 2023
@jw3
Eliminate vendoring on Rawhide (#730)
5627162 
It has become possible to eliminate crate vendoring all together for
Rawhide.

This PR removes vendoring from the top level spec, Containerfile, and CI.

This also:
- Drops fc37 build
- Bumps nom to v7.1
- Switches from lmdb-rkv to lmdb v0.8
- Bumps other deps for Fedora package compat
- Uses urls for SourceX rather than relative paths

#715
# Conflicts:
#	.copr/Makefile
#	.github/workflows/rpm.yml
#	Cargo.lock
#	Containerfile
#	crates/trust/Cargo.toml
#	crates/trust/src/load.rs
#	crates/util/Cargo.toml
#	fapolicy-analyzer.spec
#	scripts/srpm/fapolicy-analyzer.spec
jw3 added a commit that referenced this issue Jan 9, 2023
@jw3
Eliminate vendoring on Rawhide (#730)
5ab14eb 
It has become possible to eliminate crate vendoring all together for
Rawhide.

This PR removes vendoring from the top level spec, Containerfile, and CI.

This also:
- Drops fc37 build
- Bumps nom to v7.1
- Switches from lmdb-rkv to lmdb v0.8
- Bumps other deps for Fedora package compat
- Uses urls for SourceX rather than relative paths

#715
# Conflicts:
#	.copr/Makefile
#	.github/workflows/rpm.yml
#	Cargo.lock
#	Containerfile
#	crates/trust/Cargo.toml
#	crates/trust/src/load.rs
#	crates/util/Cargo.toml
#	fapolicy-analyzer.spec
#	scripts/srpm/fapolicy-analyzer.spec
This was referenced Jan 9, 2023
Closed
Merged
@jw3
Copy link
Member Author

jw3 commented Jan 10, 2023

Will resubmit the review once #733 is merged and picked back to 0.6.

jw3 added a commit that referenced this issue Jan 10, 2023
@jw3
Fix locale install (#733)
227fcce 
The application locales were not being included in the RPM. This PR
rearranges the location of locales, bringing them up to a top level
directory, and fixes up the installation process.

Adds additional content to the man page and fixes an issue installing it
#200.

This also fixes up a few annoyances caused from the vendoring refactor
in #730.

Also adds a check stage to CI that performs a RPM install and displays
the rpmdb metadata for it.

More #715
Closes #720
jw3 added a commit that referenced this issue Jan 10, 2023
@jw3
Fix locale install (#733)
93f629f 
The application locales were not being included in the RPM. This PR
rearranges the location of locales, bringing them up to a top level
directory, and fixes up the installation process.

Adds additional content to the man page and fixes an issue installing it
#200.

This also fixes up a few annoyances caused from the vendoring refactor
in #730.

Also adds a check stage to CI that performs a RPM install and displays
the rpmdb metadata for it.

More #715
Closes #720
# Conflicts:
#	.copr/Makefile
#	Makefile
#	fapolicy-analyzer.spec
#	scripts/srpm/fapolicy-analyzer.spec
jw3 added a commit that referenced this issue Jan 10, 2023
@jw3
Fix locale install (#733)
1e4e340 
The application locales were not being included in the RPM. This PR
rearranges the location of locales, bringing them up to a top level
directory, and fixes up the installation process.

Adds additional content to the man page and fixes an issue installing it
#200.

This also fixes up a few annoyances caused from the vendoring refactor
in #730.

Also adds a check stage to CI that performs a RPM install and displays
the rpmdb metadata for it.

More #715
Closes #720
# Conflicts:
#	.copr/Makefile
#	Makefile
#	fapolicy-analyzer.spec
#	scripts/srpm/fapolicy-analyzer.spec
@jw3 jw3 mentioned this issue Jan 11, 2023
Closed
@jw3
Copy link
Member Author

jw3 commented Jan 11, 2023

[!]: Spec file according to URL is the same as in SRPM.
Note: Bad spec filename: /var/lib/copr-rpmbuild/results/fapolicy-
analyzer/srpm-unpacked/fapolicy-analyzer.spec
See: (this test has no URL)

Another round of feedback. Closer, only one finding. Appears that the app source tarball that gets packaged in srpm does not match what actions is archiving. Going to use a single source across all jobs to ensure identity.

@jw3
Copy link
Member Author

jw3 commented Jan 11, 2023

image

@jw3
Copy link
Member Author

jw3 commented Jan 13, 2023

https://bugzilla.redhat.com/show_bug.cgi?id=2153687#c24

@jw3 jw3 closed this as completed Jan 13, 2023
@jw3 jw3 mentioned this issue Jan 13, 2023
Merged
jw3 added a commit that referenced this issue Jan 16, 2023
@jw3
Picks from Fedora review (#737)
51f3cd1 
Porting back a few changes to release process made in the 0.6 branch
while working on #715
jw3 added a commit that referenced this issue Jan 16, 2023
@jw3
Picks from Fedora review (#737)
d77ce87 
Porting back a few changes to release process made in the 0.6 branch
while working on #715
@jw3 jw3 unpinned this issue Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
build feedback1 rpm
Projects
None yet
Milestone
v1.0
Development

No branches or pull requests
1 participant
@jw3