Alternatives: avoid explicit paths to update-alternatives

yselkowitz commented

2024-03-21 22:07:14 +00:00

Member

Using installation path macros in file dependencies is disallowed by https://pagure.io/packaging-committee/c/35d89f0f3ff118359f1fef1c328570a944e0ddf8 ("Recommend avoiding macros in file depenencies") and breaks flatpak builds when the dependency is in the runtime.

tibbs commented

2024-03-22 15:52:11 +00:00

Owner

Extra % here

tibbs commented

2024-03-22 16:02:30 +00:00

Owner

I have long been a proponent of avoiding path macros outside of situations like %_libdir where they are required or where they enhance readability, so I'm certainly happy with this but I suppose I'm a bit biased.

I have long been a proponent of avoiding path macros outside of situations like `%_libdir` where they are required or where they enhance readability, so I'm certainly happy with this but I suppose I'm a bit biased.

yselkowitz commented

2024-03-22 16:44:25 +00:00

Author

Member

rebased onto 5940265643265b43d9877eeb71a9762680083aac

yselkowitz commented

2024-03-22 16:44:26 +00:00

Author

Member

Extra % here

Thanks, fixed.

> Extra % here Thanks, fixed.

tuliom commented

2024-03-25 21:38:12 +00:00

Member

I wonder if there is another place of the documentation that might need to be updated.
When referring to macros for paths, the documentation says:

So, it’s usually preferable to not hard-code these paths in spec files either, but use the same macros for consistency.

Source: https://docs.fedoraproject.org/en-US/packaging-guidelines/RPMMacros/#macros_installation

If I'm understanding the issue correctly, the part that I quoted may need to be removed and the previous part might need to be modified to:

... use the values defined by RPM to set installation paths for packages and when referring to files and directories installed by the current spec file.

Is that interpretation correct?

I wonder if there is another place of the documentation that might need to be updated. When referring to macros for paths, the documentation says: > So, it’s usually preferable to not hard-code these paths in spec files either, but use the same macros for consistency. Source: https://docs.fedoraproject.org/en-US/packaging-guidelines/RPMMacros/#macros_installation If I'm understanding the issue correctly, the part that I quoted may need to be removed and the previous part might need to be modified to: > ... use the values defined by RPM to set installation paths for packages **and when referring to files and directories installed by the current spec file**. Is that interpretation correct?

tstellar commented

2024-03-25 21:52:52 +00:00

Contributor

Isn't https://pagure.io/packaging-committee/c/35d89f0f3ff118359f1fef1c328570a944e0ddf8 just referring to paths in BuildRequires/Requires ?

yselkowitz commented

2024-03-25 22:21:45 +00:00

Author

Member

... use the values defined by RPM to set installation paths for packages and when referring to files and directories installed by the current spec file.

Is that interpretation correct?

Something like that, yes.

> > ... use the values defined by RPM to set installation paths for packages **and when referring to files and directories installed by the current spec file**. > > Is that interpretation correct? Something like that, yes.

yselkowitz commented

2024-03-25 22:27:25 +00:00

Author

Member

Isn't https://pagure.io/packaging-committee/c/35d89f0f3ff118359f1fef1c328570a944e0ddf8 just referring to paths in BuildRequires/Requires ?

By the same logic, if you can't rely on using e.g. Requires: %{_sbindir}/alternatives, you also can't call %{_sbindir}/alternatives in scriptlets etc. either. Besides, it doesn't make sense for the style of these two (the file dependency and its invocation in the body of a spec file) to be different.

> Isn't https://pagure.io/packaging-committee/c/35d89f0f3ff118359f1fef1c328570a944e0ddf8 just referring to paths in BuildRequires/Requires ? By the same logic, if you can't rely on using e.g. `Requires: %{_sbindir}/alternatives`, you also can't call `%{_sbindir}/alternatives` in scriptlets etc. either. Besides, it doesn't make sense for the style of these two (the file dependency and its invocation in the body of a spec file) to be different.

tstellar commented

2024-03-25 22:45:39 +00:00

Contributor

So does this mean that something like this is wrong:

https://github.com/rpm-software-management/rpm/blob/master/macros.in#L769

So does this mean that something like this is wrong: https://github.com/rpm-software-management/rpm/blob/master/macros.in#L769

yselkowitz commented

2024-03-25 23:16:48 +00:00

Author

Member

So does this mean that something like this is wrong:

https://github.com/rpm-software-management/rpm/blob/master/macros.in#L769

Since we anyway have to override PKG_CONFIG_PATH (and other such variables) to search both /app and /usr prefixes simultaneously, the question is practically moot.

> So does this mean that something like this is wrong: > > https://github.com/rpm-software-management/rpm/blob/master/macros.in#L769 Since we anyway have to override PKG_CONFIG_PATH (and other such variables) to search both /app and /usr prefixes simultaneously, the question is practically moot.

tstellar commented

2024-03-25 23:25:17 +00:00

Contributor

So does this mean that something like this is wrong:

https://github.com/rpm-software-management/rpm/blob/master/macros.in#L769

Since we anyway have to override PKG_CONFIG_PATH (and other such variables) to search both /app and /usr prefixes simultaneously, the question is practically moot.

I understand this specific case may not matter for flatpak builds, but I'm asking more in general is something like this not allowed according to the packaging guidelines.

> > So does this mean that something like this is wrong: > > > > https://github.com/rpm-software-management/rpm/blob/master/macros.in#L769 > > Since we anyway have to override PKG_CONFIG_PATH (and other such variables) to search both /app and /usr prefixes simultaneously, the question is practically moot. I understand this specific case may not matter for flatpak builds, but I'm asking more in general is something like this not allowed according to the packaging guidelines.

yselkowitz commented

2024-03-26 00:41:14 +00:00

Author

Member

I understand this specific case may not matter for flatpak builds, but I'm asking more in general is something like this not allowed according to the packaging guidelines.

The difference here is that any given individual dependency will only be in one location at a time, and the question is how to refer to that dependency in a way that applies to both regular and flatpak builds. When it comes to search paths though, an override is anyways necessary to handle multiple prefixes, and I'm not proposing that they should (since, in the case of regular builds, this would just result in duplicate entries). Therefore, I don't want to compare these use cases at all.

> I understand this specific case may not matter for flatpak builds, but I'm asking more in general is something like this not allowed according to the packaging guidelines. The difference here is that any given individual dependency will only be in one location at a time, and the question is how to refer to that dependency in a way that applies to both regular and flatpak builds. When it comes to search paths though, an override is anyways necessary to handle multiple prefixes, and I'm not proposing that they should (since, in the case of regular builds, this would just result in duplicate entries). Therefore, I don't want to compare these use cases at all.

vondruch commented

2024-04-03 07:59:03 +00:00

Member

The commit message is too terse to my taste.

churchyard commented

2024-04-04 09:18:13 +00:00

Owner

I think the change is correct and the reasons is well documented in the commit message.

yselkowitz commented

2025-02-20 13:40:48 +00:00

Author

Member

rebased onto b85d6de485

rebased onto b85d6de4858b9a826c69ca531d1c2ec2813f141e

yselkowitz commented

2025-02-20 13:41:51 +00:00

Author

Member

Rebased; how can this be moved forward?

tuliom commented

2025-02-20 14:53:31 +00:00

Member

I have reviewed this as an user and it LGTM.
This documentation works before and after the bin/sbin merge.

I have reviewed this as an user and it LGTM. This documentation works before and after the bin/sbin merge.

This pull request is broken due to missing fork information.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin flatpaks:flatpaks

git switch flatpaks

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main

git merge --no-ff flatpaks

git switch flatpaks

git rebase main

git switch main

git merge --ff-only flatpaks

git switch flatpaks

git rebase main

git switch main

git merge --no-ff flatpaks

git switch main

git merge --squash flatpaks

git switch main

git merge --ff-only flatpaks

git switch main

git merge flatpaks

git push origin main

Rows
Columns

Alternatives: avoid explicit paths to update-alternatives #1346

Checkout

Merge