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.

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

add support for spec file templates into builddep subcommand #420

Closed
wants to merge 1 commit into from

Conversation

clime
Copy link
Contributor

@clime clime commented Dec 23, 2020

Hello!

This PR adds support for spec file preprocessing (per https://fedoraproject.org/wiki/Changes/Enable_Spec_File_Preprocessing) into the builddep subcommand.

It uses preproc-rpmspec utillity (at least version 1.2) and also adds Requires on it into the spec file. The utility is at the moment on its way to stable repos for epel7, epel8 and all Fedoras (https://bodhi.fedoraproject.org/updates/FEDORA-2020-091d3f026a).

I haven't found a way to run tests, the instructions in README.rst don't seem to work (make test command errs out with `No rule to make target 'test'.)

NOTES from the commit:

  • uses preproc-rpmspec to render the spec file
  • the spec file is only preprocessed if preprocessing is
    enabled by rpkg configuration within the spec file's context
    (i.e. git-toplevel's rpkg.conf and spec's directory rpkg.conf
    are read if present), otherwise the original text is simply
    returned. This is the effect of --check-context option of
    preproc-rpmspec.

@@ -42,6 +42,7 @@ Source0: %{url}/archive/%{version}/%{name}-%{version}.tar.gz
BuildArch: noarch
BuildRequires: cmake
BuildRequires: gettext
Requires: preproc-rpmspec >= 1.2
Copy link
Member

Choose a reason for hiding this comment

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

This cannot be a mandatory dependency, as it will not be pulled into RHEL. Additionally, other distributions will not necessarily have this tool packaged either.

Please rework so that this is an optional dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review. I did the changes. Please, take a look.

@clime
Copy link
Contributor Author

clime commented Dec 24, 2021

In retrospect, I don't consider the changes made after @Conan-Kudo review to be good. If this is ever considered for merge, the code should be revisited.

@clime
Copy link
Contributor Author

clime commented Dec 24, 2021

In retrospect, I don't consider the changes made after @Conan-Kudo review to be good. If this is ever considered for merge

So, please, let me know when that might be the case and I will revisit this, otherwise feel free to close it.

@Conan-Kudo
Copy link
Member

Note that dnf-plugins-core is part of RHEL, and there's basically no way preproc-rpmspec would be pulled into RHEL, so it needs to function without it.

@clime
Copy link
Contributor Author

clime commented Dec 24, 2021

Note that dnf-plugins-core is part of RHEL, and there's basically no way preproc-rpmspec would be pulled into RHEL, so it needs to function without it.

Ok, but changing behavior just based on some tool being installed or not doesn't feel right to me so that's why I think this would require further look.

To explain a bit more: There is already change of behavior based on whether the target spec file is in git repo with rpkg.conf stating that preprocessing is allowed, which I don't consider the greatest feature but somewhat acceptable given the options but conditioning preprocessing enabled/disabled by preproc-rpmspec being installed would be bad for user experience, I feel.

@Conan-Kudo
Copy link
Member

Conan-Kudo commented Dec 24, 2021

Basically, it needs to gracefully fail if the tool isn't installed. As long as that's the case, I think we can review including it. Then you can make the EPEL package supplement dnf-plugins-core.

@clime
Copy link
Contributor Author

clime commented Dec 24, 2021

Can you explain what you mean by "gracefully fail"?

I guess, as a user, I would expect the behavior such that if something is enabled but the tool isn't installed, then fail.
But if something isn't enabled and the tool isn't installed, then it's ok.

Is it what you have in mind?

If that's the case, the problem is that the logic that decides whether the thing is enabled is in the tool itself. I think the only way out would be to have some extra dnf config option, which would explicitly say whether the feature is enabled or not...

@pep8speaks
Copy link

pep8speaks commented Dec 25, 2021

Hello @clime! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-12-25 19:48:13 UTC

* uses preproc-rpmspec to render the spec file
* usage of --spec switch is required for spec.rpkg files
* the spec file is only preprocessed if preprocessing is
  enabled by rpkg configuration within the spec file's context
  (i.e. git-toplevel's rpkg.conf and spec's directory rpkg.conf
  are read if present), otherwise the original text is simply
  returned. This is the effect of --check-context option of
  preproc-rpmspec.
@clime
Copy link
Contributor Author

clime commented Dec 25, 2021

I returned to the original form of this pull request so this doesn't hang here in the shape it was. Feel free to close it you don't see way forward for it.

@kontura kontura self-assigned this Mar 30, 2023
@kontura
Copy link
Contributor

kontura commented Apr 3, 2023

Based on the previous discussion of problems and the fact the dnf5 will replace current dnf in fedora 39 making this change quite short lived, I am closing the PR as it is probably not worth the effort.
The functionality could be added to dnf5 builddep build instead.

@kontura kontura closed this Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants