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 special system-level %include path #685

Closed

Conversation

pmatilai
Copy link
Member

When %include argument is enclosed in <> (ie similar to C includes), look for the file in system-level %_rpmincludedir path instead %_sourcedir, and treat these as build-dependencies that get recorded
in the src.rpm.

Depends on #684

On forced spec parse (spec query or --force'd build) we already allow
sources and patches to be missing. %include differs from sources and
patches in that a missing include might make the spec unparseable,
but then we lose nothing by trying, as quite often the spec is parseable
enough to get at least some info out of it. Doesn't seem any worse
than allowing build without sources present...

Helps a bit in cases like RhBug:1658292 and RhBug:1547897.
When %include argument is enclosed in <> (ie similar to C includes),
look for the file in system-level %_rpmincludedir path instead
%_sourcedir, and treat these as build-dependencies that get recorded
in the src.rpm.
*(endFileName-1) = '\0';
sysinc = rpmGetPath("%{_rpmincludedir}/", fileName + 1, NULL);
addReqProv(spec->sourcePackage, RPMTAG_REQUIRENAME,
sysinc, NULL, 0, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this one, because you are technically do not depend on exact /usr/lib/rpm/include/… file, but rather that path will be present...

Copy link
Member Author

Choose a reason for hiding this comment

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

The point is that other tooling knows how to look at buildrequires, but they dont (and should not) try to parse rpmbuild output for missing files.

@voxik
Copy link
Contributor

voxik commented Apr 26, 2019

I wish %_rpmincludedir was externaly configurable somehow, e.g. by env variable or cmd line option. Otherwise it will make life harder to SCLs, where everything should go into /opt.

What is the use case anyway? I can imagine using this for including pre-generated changelogs, but again, it would need to be configurable ...

@pavlinamv
Copy link
Contributor

Please can you add a test case that ends successfully?

Note that:
After this commit there will be two types of %include

  • basic "%include file" and
  • require/provides from system-level dir "%include ",
    If there will be a need for another special type of %include in future, it will be hard to create another type of notation.

@pmatilai pmatilai added the RFC label Apr 29, 2019
@pmatilai
Copy link
Member Author

Just to make it clear, this is more for discussion about the feature and not something to be merged just because it's technically correct.

@pmatilai
Copy link
Member Author

pmatilai commented Apr 29, 2019

Please can you add a test case that ends successfully?

Lets see where the discussion goes. Creating that case is rather tedious as the included file needs to be packaged for the test to work.

Note that: After this commit there will be two types of %include

Well, creating a second type of include is kinda the whole point.

@pmatilai
Copy link
Member Author

I wish %_rpmincludedir was externaly configurable somehow, e.g. by env variable or cmd line option. Otherwise it will make life harder to SCLs, where everything should go into /opt.

It's a macro. You can --define it to whatever you like.

What is the use case anyway? I can imagine using this for including pre-generated changelogs, but again, it would need to be configurable ...

Sorry, I assumed it was kinda obvious: to share whatever common pieces between packages in an ecosystem, to put it broadly. An ecosystem could be a programming language, or extension to a specific program/library, etc. Currently such things are stuffed into macro files in /usr/lib/rpm/macros.d/ but there are various problems with this:

  • there's no build-require trace of where those macros come from
  • macro files have limitations that spec includes don't (eg conditionals)
  • those macros are loaded on every single rpm invocation for no good reason

So the idea is that, say, Python 3 package on a distro would do something like
%include <python3.inc>
...which will automatically create a build-dependency on the path where that file comes from - presumably python3-devel. And even if we skipped the automatic dependency, it creates a strong trace as to what macros this package needs. Much more so than the current automatically loaded stuff does.

@voxik
Copy link
Contributor

voxik commented Apr 29, 2019

I wish %_rpmincludedir was externaly configurable somehow, e.g. by env variable or cmd line option. Otherwise it will make life harder to SCLs, where everything should go into /opt.

It's a macro. You can --define it to whatever you like.

Oh, yes, probably. But is there way to extend it? E.g.:

$ rpm --define "_rpmconfigdir %{_rpmconfigdir}:foo" -E "%{_rpmconfigdir}"
error: Too many levels of recursion in macro expansion. It is likely caused by recursive macro declaration.

What is the use case anyway? I can imagine using this for including pre-generated changelogs, but again, it would need to be configurable ...

Sorry, I assumed it was kinda obvious: to share whatever common pieces between packages in an ecosystem, to put it broadly. An ecosystem could be a programming language, or extension to a specific program/library, etc. Currently such things are stuffed into macro files in /usr/lib/rpm/macros.d/ but there are various problems with this:

* there's no build-require trace of where those macros come from

* macro files have limitations that spec includes don't (eg conditionals)

* those macros are loaded on every single rpm invocation for no good reason

So the idea is that, say, Python 3 package on a distro would do something like
%include <python3.inc>
...which will automatically create a build-dependency on the path where that file comes from - presumably python3-devel. And even if we skipped the automatic dependency, it creates a strong trace as to what macros this package needs. Much more so than the current automatically loaded stuff does.

That makes sense. But while "those macros are loaded on every single rpm invocation for no good reason", then every Python .spec file would need to have this include. It is not necessarily wrong ...

@pmatilai
Copy link
Member Author

pmatilai commented Apr 29, 2019

Oh, yes, probably. But is there way to extend it? E.g.:

No, this is not a search path, it's just a single directory. I'm not sure a search path is (reasonably) doable in the spec parser realm.

That makes sense. But while "those macros are loaded on every single rpm invocation for no good reason", then every Python .spec file would need to have this include. It is not necessarily wrong ...

That's exactly the intention! Like I noted in an earlier comment, the current scheme is fundamentally flawed and this is an attempt to improve it. Take any programming language you like, and imagine it without explicit include/import/use statements and contemplate on that for a while 🙂

FWIW, alternatively (or additionally) we could have a similar syntax to load system macro files instead of spec snippets. Macro files are a much more predictable format that can actually be validated, and this would allow (re)use of the existing macro files in distros instead of having to convert them to spec syntax.

@pmatilai
Copy link
Member Author

Hmm, the more I think about it, the more favorable going for macro-file includes (instead of spec parts) seems. Macro files can of course already be loaded with %load, but that's a low-level macro-primitive, for spec we'd want something more high-level that can also affect buildrequires etc. Could also be a nice opportunity to start introducing namespaces to macros...

@voxik
Copy link
Contributor

voxik commented Apr 30, 2019

Thinking about this, I am not sure this provides any benefit. If I have to have the include explicitly in the .spec file, then I don't know why I should not have it including full path, because this must be documented somewhere anyway. Why for example Ruby should put some "include" file into RPM directory and not keep it somewhere else?

@pmatilai
Copy link
Member Author

Why does every single programming language have a built-in path for looking up includes/imports instead of using a hardcoded path?

@voxik
Copy link
Contributor

voxik commented Apr 30, 2019

Why does every single programming language have a built-in path for looking up includes/imports instead of using a hardcoded path?

But then every single programming language allows to extend the search paths, which is not part of this proposal apparently.

Also every programming language supports portability to different platforms, but the %includes in RPM world are IMO bound to the distribution, so there is no need to fiddle with the paths, because they are pretty static IMO.

@pmatilai
Copy link
Member Author

Yes, but we still allow changing the path. Also, part of the reason to have a special syntax is to enable that build-dependency tracking. If it's just %include /some/path/somewhere rpm cannot make assumptions about it being packaged.

@voxik
Copy link
Contributor

voxik commented Apr 30, 2019

rpm cannot make assumptions about it being packaged.

I could imagine that assumption about being available/installed was useful (but then the file would have to be shipped by RPM itself), but I can't see what is this assumption good for?

@pmatilai pmatilai closed this May 6, 2019
@pmatilai pmatilai deleted the rpminclude-pr branch May 6, 2019 09:53
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 this pull request may close these issues.

None yet

4 participants