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

Cannot leave comments after %endif #829

Open
ernestask opened this issue Sep 4, 2019 · 10 comments
Open

Cannot leave comments after %endif #829

ernestask opened this issue Sep 4, 2019 · 10 comments

Comments

@ernestask
Copy link
Contributor

@ernestask ernestask commented Sep 4, 2019

#625 broke a rather reasonable use case, i.e.

%if %{with foo}
...
%else # with foo

Not even our friendly neighborhood 44180fc can be used.

What would be the proposed replacement for that, one that doesn’t disconnect the macro with the comment?

@pavlinamv

This comment has been minimized.

Copy link
Contributor

@pavlinamv pavlinamv commented Sep 4, 2019

From Fedora documentation [1]: "To include comments in the spec file, use a # character at the start of the line."
So the text "# with foo" in line
%else # with foo
is not a valid comment. If you need to use a comment near %else, you can write e.g.:

%if %{with foo}
...
%else
# with foo

[1] https://docs.fedoraproject.org/en-US/Fedora_Draft_Documentation/0.1/html/Packagers_Guide/chap-Packagers_Guide-Spec_File_Reference-Comments.html

@ernestask

This comment has been minimized.

Copy link
Contributor Author

@ernestask ernestask commented Sep 4, 2019

But that’s just Fedora documentation, not RPM documentation, so it’s really you who sets the restrictions, because you own the parser.

And notice how I mentioned not disconnecting the comment, because, if you do that, you introduce additional overhead when moving/removing. One has to be more careful to not leave a stray # with foo comment without an %endif macro to accompany it.

Regardless, we still have the %dnl macro, but now we’re getting into the territory of special-casing things.

@ernestask

This comment has been minimized.

Copy link
Contributor Author

@ernestask ernestask commented Sep 4, 2019

Okay, my bad here. I was getting build errors, but seems that it’s because a test was failing prior. The output confused me.

@mmathesius

This comment has been minimized.

Copy link

@mmathesius mmathesius commented Sep 6, 2019

I'd really like to see this addressed.

I recently started getting such warnings from a package spec I maintain. It has several levels of nested %ifs and cases where the %if and corresponding %else/%endif are separated by many lines. It's really helpful to be able to match up the conditionals with a simple connected comment. Even the C source code for the spec parser that is reporting this warning makes use of a comment on an #endif line!

Also, note that the above referenced Fedora documentation describing spec file comments goes on to mention "inline comments"--so somebody was thinking about comments that didn't start at the beginning of a line.

@ernestask

This comment has been minimized.

Copy link
Contributor Author

@ernestask ernestask commented Sep 6, 2019

To be fair, the C grammar is such that the comments are completely discarded by the lexer, if I’m not mistaken. However, I could not find any official formal grammar for spec files, so I’m thinking that it’s just cobbled together to make things work. The problem is exacerbated by shell commands in certain sections, which allow you to, say, create an ad-hoc shell script with a shebang. Is that a comment as far as the lexer/parser/whatever is concerned? Seems rather context-dependent, but there might be more corner cases like this.

@pmatilai

This comment has been minimized.

Copy link
Contributor

@pmatilai pmatilai commented Sep 9, 2019

Rpm has only ever supported comments at the beginning of a spec line, ie lines starting with #, but like @ernestask points out, much of the spec is actually shell-script where comments are allowed anywhere in the line so it's easy to get mixed up with expectations.

Anything else appearing to work as a comment has been accidental, as in: bugs in the spec parser,
see the discussion in #625 for a freak show of side-effects that this particular bug has allowed. Also note that what was added is just a warning, if a build is failing it's failing for some other reason.

Eliminating such quirks is in everybody's best interest, but I agree in this case it outlaws a useful pattern. The constructive solution to the problem would be supporting spec #-comments at arbitrary positions, just like the shell does.

@pavlinamv

This comment has been minimized.

Copy link
Contributor

@pavlinamv pavlinamv commented Sep 10, 2019

The constructive solution to the problem would be supporting spec #-comments at arbitrary positions, just like the shell does.

Supporting spec #-comments at arbitrary positions can potentially cause some problems. That is why I looked into spec files and search for occurrences of #. There are several types of interesting lines:

1/ # used in define (in quotes or before backslash, ...)

   %define coolkey_module "CoolKey PKCS #11 Module"
   %global auto_register_macro_post() # create it if it doesn't already exist \

2/ # used in a script that does not denote a comment

sed -i -e '/^#!/ c #!%{__perl}' bin/podspell
%{_bindir}/echo -e "\e[101m -=#=- Tests disabled -=#=- \e[0m"

3/ # used in a changelog:

Ticket #321 - krbExtraData is being null modified and replicated on each ssh login
@pmatilai

This comment has been minimized.

Copy link
Contributor

@pmatilai pmatilai commented Sep 11, 2019

Never said it'd be easy to retrofit. But basically

  1. Macros are handled by the macro engine, not spec. The macro body is always literal and must remain so, so that auto_register_macro_post() macro would essentially emit a #-comment line.
  2. Comments in build-scriptlets are shell comments, not rpm's concern.
  3. Do comments belong to changelog at all? I don't think so... we could probably simply disable them there entirely and nobody would notice.
@ernestask

This comment has been minimized.

Copy link
Contributor Author

@ernestask ernestask commented Sep 11, 2019

As far as 3 goes, does anything other than text belong there?

@pavlinamv

This comment has been minimized.

Copy link
Contributor

@pavlinamv pavlinamv commented Sep 16, 2019

  1. agree with @pmatilai, changelog entries can be without comments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.