Bug 1224660 - Macros shouldn't expand in comments
Summary: Macros shouldn't expand in comments
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: rpm
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Packaging Maintenance Team
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-05-25 09:23 UTC by Jaroslav Škarvada
Modified: 2023-06-28 12:40 UTC (History)
14 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-08-28 11:53:17 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)

Description Jaroslav Škarvada 2015-05-25 09:23:25 UTC
Description of problem:
I think conditionals, global defines and bconds shouldn't expand in comments. 

Version-Release number of selected component (if applicable):
rpm-4.12.0.1-6.fc21.x86_64

How reproducible:
Always

Steps to Reproduce:
1. Create spec with the following content:
#%ifarch %{ix86} x86_64
#%bcond_without openblas
#%else
 %bcond_with openblas
#%endif

Actual results:
Build with openblas on x86

Expected results:
Build without openblas on x86

Additional info:
The current behaviour is confusing. User may forget to escape macros in comments by second %. Maybe that no macro should expand in comments, or is there any use case for it?

Comment 1 Ľuboš Kardoš 2015-06-12 15:51:18 UTC
We don't change this behaviour right now because somebody can depend on it. But we added a warning that a macro is expanded in comment. So user will be aware that he has to escape macros also in comments if he don't want them to be expanded.

Fixed upstream as 2a3f49585e5bd82d0bbfe5b9d6cdf24d9501b5cd.

Comment 2 Vít Ondruch 2015-07-30 09:39:05 UTC
(In reply to Ľuboš Kardoš from comment #1)
> We don't change this behaviour right now because somebody can depend on it.
> But we added a warning that a macro is expanded in comment. So user will be
> aware that he has to escape macros also in comments if he don't want them to
> be expanded.
> 
> Fixed upstream as 2a3f49585e5bd82d0bbfe5b9d6cdf24d9501b5cd.

This is regression:

varování: Macro expanded in comment on line 16: # Executing testsuite (enabling %%check section) will cause dependency loop.

It was always enough to prepend another % in front of the macro to avoid the warning. Now this is apparently ignored.

With single %, rpmlint always fired warning:

$ rpmlint -i rubygems.spec
rubygems.spec:16: W: macro-in-comment %check
There is a unescaped macro after a shell style comment in the specfile. Macros
are expanded everywhere, so check if it can cause a problem in this case and
escape the macro with another leading % if appropriate.

Comment 3 Vít Ondruch 2015-07-30 09:40:00 UTC
(In reply to Ľuboš Kardoš from comment #1)
> Fixed upstream as 2a3f49585e5bd82d0bbfe5b9d6cdf24d9501b5cd.

Btw since you include hash of the commit, it would be more useful to sent a link instead. That way I could immediately review your change.

Comment 4 Jaroslav Škarvada 2015-07-30 10:38:53 UTC
(In reply to Ľuboš Kardoš from comment #1)
> We don't change this behaviour right now because somebody can depend on it.
> But we added a warning that a macro is expanded in comment. So user will be
> aware that he has to escape macros also in comments if he don't want them to
> be expanded.
> 
> Fixed upstream as 2a3f49585e5bd82d0bbfe5b9d6cdf24d9501b5cd.

I can't imagine any use case for macro expansion in comments. Is there any? If not, I think this could be good feature candidate for e.g. rpm 5.

Comment 5 Ľuboš Kardoš 2015-07-31 08:37:15 UTC

(In reply to Vít Ondruch from comment #2)
> (In reply to Ľuboš Kardoš from comment #1)
> > We don't change this behaviour right now because somebody can depend on it.
> > But we added a warning that a macro is expanded in comment. So user will be
> > aware that he has to escape macros also in comments if he don't want them to
> > be expanded.
> > 
> > Fixed upstream as 2a3f49585e5bd82d0bbfe5b9d6cdf24d9501b5cd.
> 
> This is regression:
> 
> varování: Macro expanded in comment on line 16: # Executing testsuite
> (enabling %%check section) will cause dependency loop.
> 
> It was always enough to prepend another % in front of the macro to avoid the
> warning. Now this is apparently ignored.
> 
> With single %, rpmlint always fired warning:
> 
> $ rpmlint -i rubygems.spec
> rubygems.spec:16: W: macro-in-comment %check
> There is a unescaped macro after a shell style comment in the specfile.
> Macros
> are expanded everywhere, so check if it can cause a problem in this case and
> escape the macro with another leading % if appropriate.

Now the warning is not showed when a macro is escaped by adding another % in front of the macro. Fixed in version rpm-4.12.90-4.fc24. (upstream: https://github.com/rpm-software-management/rpm/commit/655ca18630d4a89e022b51ea495fa63718303413)

Comment 6 Panu Matilainen 2015-11-23 07:51:04 UTC
This produces many unnecessary warnings, since read-only expansion is not harmful whether in comments or not. Defining (or undefining) macros in comments is the nasty part, but of course there's no way for the spec parser to know that.

My (rather vague) plan was to add some sort of read/write/execute mode flags to allow controlling what the macro engine is allowed to do at a given time. For spec parsing it could drop to read-only for apparent comment lines (and warn/error on define/undefine attempt). There are other places that could benefit from restricted macro execution too.

Comment 7 Panu Matilainen 2015-11-23 07:53:12 UTC
(In reply to Panu Matilainen from comment #6)
> Defining (or undefining) macros in comments is the nasty part, but of course
> there's no way for the spec parser to know that.

s/know that/know what an arbitrary macro might do/

Comment 8 Jean Delvare 2018-12-23 11:41:37 UTC
(In reply to Ľuboš Kardoš from comment #1)
> We don't change this behaviour right now because somebody can depend on it.

How could somebody actually depend on this buggy behavior? Do you know of any real world situation?

I can easily think of situations where this behavior will cause trouble (the most obvious one: you comment out a macro definition and the macro is still defined - if I understand correctly). But I can't think of any case where someone would actually want the current behavior.

The whole purpose of comments in "source code" is to be read by humans and ignored by machines. The fact that rpm is parsing comments at all is simply wrong.

Comment 9 Panu Matilainen 2019-06-27 09:37:11 UTC
FWIW, rpm >= 4.15 has a real commenting primitive in the way of M4-inspired %dnl (discard to next line). As the name implies, it simply discards anything to the next newline, and macros in the discarded part are naturally not expanded.

See https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/7Q3BZ6GTAKFDJMNOLNJJENMWCPQTTET7/ and https://github.com/rpm-software-management/rpm/commit/44180fc7400ea8e67a13c5581459ad13f1d14103 for more info.

Comment 12 Tom Hiddleston 2022-12-21 09:49:15 UTC Comment hidden (spam)
Comment 13 adamusa 2023-06-27 07:27:26 UTC Comment hidden (spam)

Note You need to log in before you can comment on or make changes to this bug.