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

Indentation support for tags #2927

Closed
Root-Core opened this issue Feb 22, 2024 · 9 comments · Fixed by #2957
Closed

Indentation support for tags #2927

Root-Core opened this issue Feb 22, 2024 · 9 comments · Fixed by #2957
Assignees

Comments

@Root-Core
Copy link

Describe the bug

I try to implement a cross distribution spec file and the dependency names differ.
So I added an if and it works fine, until I indent the line in the if.

To Reproduce

  1. Store the spec file in /tmp/test.spec
  2. run rpm --specfile /tmp/test.spec
Name:       demo
Version:    1.0.0
Release:    0
Summary:    RPM package
License:    GPL
BuildArch:  noarch

%if 0%{?suse_version}
  Requires: xdotool
%else
  Requires: libxdo
%endif

%description
This is a test.

%install
%__install -m 755 mybinary -Dt %{buildroot}%{_bindir}/

%files
%{_bindir}/mybinary

Expected behavior

It should be fine and output demo-1.0.0-0.noarch.

Output

error: line 11: Unknown tag:   Requires: libxdo
error: query of specfile /tmp/test.spec failed, can't parse

Environment

  • OS / Distribution: tested on Alma and openSUSE
  • Version 4.16.1.3 / 4.14.3

Additional context

I am a total rpm newbie and this might be the desired behavior.
I know that the spec file "expands" / is evaluated to something with an randomly indented line, but it looks way better in the spec file.

ffesti added a commit to ffesti/rpm that referenced this issue Mar 11, 2024
Update to the documentation is still needed

Resolves: rpm-software-management#2927
ffesti added a commit to ffesti/rpm that referenced this issue Mar 11, 2024
Update to the documentation is still needed

Resolves: rpm-software-management#2927
@voxik
Copy link
Contributor

voxik commented Mar 11, 2024

I am against this request. The .spec file is not exactly easy to parse and this won't improve the situation. Also, it is kind of relieving that I don't have to bother with "proper" nesting inside conditionals.

BTW what would impact on the shorthand such as:

{?suse_version:Requires: xdotool}
{!?suse_version:Requires: libxdo}

I think the space is currently not allowed in this construct

@ffesti
Copy link
Contributor

ffesti commented Mar 11, 2024

Not sure why your personal taste would have any weight in this discussion. Neither does this change require proper nesting from anyone nor is white space disallowed in these macro expressions (as one can easily check with rpm -E).

@Root-Core
Copy link
Author

It would just be more consistent in my opinion, as everything (?) else works indented in an if block.
For me it would be a relief if the parsing (of any human-readable file) was consistent in this matter. There should always be a reason for an exception, and to my knowledge this is not the case.

On the other hand, I'm not a maintainer and have no experience in building RPMs, as this was my first attempt.

@voxik
Copy link
Contributor

voxik commented Mar 11, 2024

Not sure why your personal taste would have any weight in this discussion

Maybe because you value community feedback?

Neither does this change require proper nesting from anyone

Actually RPM requires proper nesting now (no nesting) and it is mostly fine. Allowing nesting will open the discussion about proper nesting, if it should be e.g. 2 spaces or single tab etc. It will motivate people to adjust nesting by conditions, making diffs bigger.

No choice is sometimes better.

nor is white space disallowed in these macro expressions (as one can easily check with rpm -E).

Not everything can be tested by rpm -E probably:

$ rpmbuild -D "_sourceddir ." -D "_srpmdir ." -bs newpackage.spec
setting SOURCE_DATE_EPOCH=1705363200
Wrote: /home/vondruch/rpmbuild/SRPMS/newpackage-1-1.fc41.src.rpm

vs:

$ rpmbuild -D "_sourceddir ." -D "_srpmdir ." -bs newpackage.spec.space
error: line 150: Unknown tag:  BuildRequires: foo

and this is the difference:

$ diff -u newpackage.spec{,.space}
--- newpackage.spec	2024-03-11 14:59:41.476458832 +0100
+++ newpackage.spec.space	2024-03-11 14:59:37.510442485 +0100
@@ -147,7 +147,7 @@
 Version: 1
 Release: 1%{?dist}
 License: MIT
-%{!?foo:BuildRequires: foo}
+%{!?foo: BuildRequires: foo}
 
 %description
 

@voxik
Copy link
Contributor

voxik commented Mar 11, 2024

If the nesting for preamble was allowed, would be nestable also other sections, such as %describtion, %pre, etc?

BTW this change would break at minimum Vim syntax highlighter.

@ffesti
Copy link
Contributor

ffesti commented Mar 11, 2024

The situation wrt other sections is a bit more complicated. RPM itself does not really support indentation in most. Instead for most sections (scripts and scriptlets) it just does macro expansion and #if magic and then hands the result to some interpreter - or sticks it into a tag to be handed to an interpreter later. So basically each section has it's own syntax. E.g. %sources and %patches will happily add any indentation to the front of the file name. So we are talking about the Preamble here only.

And yes, some syntax highlighting plugins would need to be adjusted.

@voxik
Copy link
Contributor

voxik commented Mar 11, 2024

The situation wrt other sections is a bit more complicated. RPM itself does not really support indentation in most. Instead for most sections (scripts and scriptlets) it just does macro expansion and #if magic and then hands the result to some interpreter - or sticks it into a tag to be handed to an interpreter later. So basically each section has it's own syntax. E.g. %sources and %patches will happily add any indentation to the front of the file name. So we are talking about the Preamble here only.

Yes, section content is section content. But I think that this is not possible: %description (note the space before %):

error: line 101: Unknown tag:  %description

and I might be wrong, but I don't see this supported by the PR. Or at least I can't see this covered by test case.

Also I am not sure how the %package foo would work. RPM does not report any error ATM and I suspect that such line is assumed to be part of earlier %description in my case, because adding %description foo afterwards, I receive error:

error: line 106: %description foo: package newpackage-foo does not exist

@voxik
Copy link
Contributor

voxik commented Mar 11, 2024

Just FTR, I keep asking because example like this:

https://src.fedoraproject.org/rpms/nodejs20/blob/3391b85e233fb582fff9471c23788df5ad582d21/f/nodejs20.spec#_156-162

What could be nested here if the PR was accepted?

ffesti added a commit to ffesti/rpm that referenced this issue Mar 14, 2024
ffesti added a commit to ffesti/rpm that referenced this issue Mar 14, 2024
pmatilai pushed a commit that referenced this issue Mar 14, 2024
@ffesti
Copy link
Contributor

ffesti commented Mar 14, 2024

Added some docs so you can find out things like that.

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

Successfully merging a pull request may close this issue.

3 participants