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

autoconf Dependency Fix #1764

Open
wants to merge 1 commit into
base: 3.x
Choose a base branch
from
Open

autoconf Dependency Fix #1764

wants to merge 1 commit into from

Conversation

jcsiadal
Copy link
Contributor

Fixes missing install dependencies.
Also made some formatting changes to the specfile.

Signed-off-by: jcsiadal <jeremy.c.siadal@intel.com>
@github-actions
Copy link

Test Results

  7 files  +  3    7 suites  +3   0s ⏱️ ±0s
43 tests +26  43 ✔️ +26  0 💤 ±0  0 ±0 
44 runs  +26  44 ✔️ +26  0 💤 ±0  0 ±0 

Results for commit d9480fa. ± Comparison against base commit 1dc6455.

%if 0%{?suse_version} || 0%{?sle_version}
Requires: perl-base
%else
Requires: perl-interpreter
Copy link
Member

Choose a reason for hiding this comment

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

The perl-base or perl-interpreter explicit dependency seems unnecessary. perl(File::Compare) pulls in those packages on each platform already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked through the dependency chain and it doesn't appear that RHEL's perl-file-compare will install the perl binary. I also looked at the Fedora autoconf specfile and they explicitly add perl-interpreter. The SUSE one isn't needed, but I wanted to keep things parallel.

Copy link
Member

Choose a reason for hiding this comment

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

If I do dnf -y install "perl(File::Compare)" perl-interpreter is pulled in on 9.1.

Copy link
Contributor Author

@jcsiadal jcsiadal Mar 31, 2023

Choose a reason for hiding this comment

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

"you SHOULD NOT assume any other packages are present as RPM dependencies and anything brought into the buildroot by the build system can change over time."

That's from Red Hat's packaging guidelines. They do say that one can usually drop Requires: if we're running autorequires, but I thought we're only doing manual deps. The other question to ask: if it's not needed, why does Fedora include it in their autoconf spec?


%install
make %{?_smp_mflags} DESTDIR=$RPM_BUILD_ROOT install

# remove share/info/dir to avoid conflict with other package installs
rm -f $RPM_BUILD_ROOT/%{install_path}/share/info/dir

%{__mkdir_p} ${RPM_BUILD_ROOT}/%{_docdir}
mkdir -p ${RPM_BUILD_ROOT}/%{_docdir}
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm surprised you would ask. Red Hat's RPM guide (at least one of them) says not to use (LSB) shell command macros. They were put in to make RPM portable across OSs (like Windows or BSD) and that never happened. They're just extra work with no benefit. SUSE says to never use them.
When I touch an RPM spec, I try and clean it up to current RPM standards, i.e. both RH and SUSE mandate %license now.

@adrianreber
Copy link
Member

Also made some formatting changes to the specfile.

If you want to make formatting changes please add them to separate commits and do not mix functional changes with formatting changes.

Copy link

A friendly reminder that this PR had no activity for 30 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants