Add "local_generator" #2734

voxik · 2023-10-24T14:49:25Z

This generator can be used by .spec file, which ships their own generators:

Source1: generator.req
%global __local_generator_requires bash %{SOURCE1}

Resolves #782

pmatilai · 2023-10-24T15:18:59Z

I have to say, there's beauty to the simplicity of this. Would be even simpler if the new generator was added as the last thing to the array I think.

Subject to name-bikeshedding of course. "local_generator" is not a bad for what it is, but my head keeps coming up with spec_somethings instead (and then rejecting) 😅

voxik · 2023-10-24T16:50:52Z

I have to say, there's beauty to the simplicity of this. Would be even simpler if the new generator was added as the last thing to the array I think.

That is an option. Will look at it tomorrow

Subject to name-bikeshedding of course. "local_generator" is not a bad for what it is, but my head keeps coming up with spec_somethings instead (and then rejecting) 😅

This is the hardest think, right :(

voxik · 2023-10-24T16:54:38Z

Subject to name-bikeshedding of course. "local_generator" is not a bad for what it is, but my head keeps coming up with spec_somethings instead (and then rejecting) 😅

This is the hardest think, right :(

And that is why I have also considered the find name, which was already used in this context. But of course this might be confusing ...

pmatilai · 2023-10-25T06:53:47Z

The name should imply this is a per-spec thing, so I don't think "find" is good. "local" is much better, but in rpm context I tend to associate that with "this host" rather than per package.

"spec" seems accurate, because it is a per-spec thing. But then that makes me think of dependencies of the spec itself. Which may even be a thing one day (technically, they do have parse-requirements which often is different from build-requirements).

How about just "package"? (%__package_requires, %__package_provides etc). That seems very much to the point too: these are specific to this package.

pmatilai · 2023-10-25T07:07:52Z

Hmm, but just "package" kinda gives the idea that these are the only dependencies this package will have, which is not the case. "package_local" maybe?

voxik · 2023-10-25T09:51:16Z

I have to say, there's beauty to the simplicity of this. Would be even simpler if the new generator was added as the last thing to the array I think.

Done. On top of that, I have also added a test case. The other generator abuses script.attr AFAICT. Or shell the other test cases be updated to not abuse script.attr anymore? 🤔

I have still left the local_generator name around, because it seems there is no clear winner yet

voxik · 2023-10-25T09:52:14Z

build/rpmfc.c

 	    char *bn = basename(files[i]);
 	    bn[strlen(bn)-strlen(".attr")] = '\0';
 	    fc->atypes[i] = rpmfcAttrNew(bn);
 	}
+	fc->atypes[nattrs - 1] = rpmfcAttrNew("local_generator");


The nfiles could be used here instead, but I am not sure it would make the code more readable.

mizdebsk · 2023-10-25T13:18:39Z

If local_generator.attr file exists then local_generator created twice. Why not simply create an empty local_generator.attr file instead?

pmatilai · 2023-10-26T07:00:27Z

I'd prefer a name that indicates the spec/package specifity somehow. @ffesti , @dmnks , thoughts, other ideas?

And yeah adding that empty file with just a comment on the purpose would actually be even more simple.

voxik · 2023-10-27T09:08:35Z

If local_generator.attr file exists then local_generator created twice.

This is good point. Not sure if this is real problem though.

Why not simply create an empty local_generator.attr file instead?

I have proposed this earlier in #782 (comment) and put this idea into practice for Fedora. I have no preference.

I just wanted to move this forward a bit ;) now I stand by waiting for guidance (i.e. the right implementation and the acceptable name).

ffesti · 2024-01-17T14:57:17Z

OK, just hard coding one file attribute doesn't seem like a good idea. I added a macro that allows you to register an arbitrary number of local file attributes and generators. I am still wondering if we need a mechanism to avoid executing generators twice when the package is already installed while a new version is building. May be we should use the original name and over write the locally installed configuration. This would be pretty simple to do I guess by just checking the list of previously loaded file attributes.

voxik · 2024-01-18T08:43:34Z

tests/rpmbuild.at

@@ -866,6 +866,7 @@ RPMTEST_CHECK([
 RPMDB_INIT

 runroot rpmbuild -bb --quiet \
+		--define '__local_file_attrs local_generator' \


While this is "just test case", maybe the local_generator could use different name, just to avoid the impression that the local is somehow mandatory in the latter

Maybe something like totally_random_generator_name

voxik · 2024-01-18T08:51:36Z

OK, just hard coding one file attribute doesn't seem like a good idea. I added a macro that allows you to register an arbitrary number of local file attributes and generators.

Nice, now we can argue if the __local_file_attrs is the right name or rather e.g. __package_local_file_attrs 🙈🤣 Sorry, couldn't resist.

Nevertheless, thx for contributing. I see your point. OTOH, if I read your commit correctly, my original proposal had the advantage, where one could count with convention over configuration. IOW with your changes one additional line is needed to make this work.

voxik · 2024-01-18T09:04:04Z

On the second thought, maybe it really is the right way. I am coming from place where no "local" generator us was possible, so one was already great improvement. But maybe there are use cases for multiple generators?

OTOH, one could call them via some helper script or by chaining them (in the worst case by executing bash or what not).

ffesti · 2024-01-18T09:07:27Z

Hmm, I guess we are taking about two different use cases. You are thinking about a dependency generator that is in the package only and not installed ever. For that you want a unique name that won't ever clash with installed file attributes.

I am thinking about actually shipping the dependency generator but also using it in the package itself. For that you need to make sure that it is only run once during the build - even if the package is actually installed in the system building the package. The you'd want the names to be the same and RPM should overwrite the installed one with the one in the package.

I guess we do want both use cases to work.

voxik · 2024-01-18T09:38:35Z

I am thinking about actually shipping the dependency generator but also using it in the package itself.

This ^^ actually is my use case and here is how it looks in practice:

https://src.fedoraproject.org/rpms/ruby/blob/308b2c0ab2c6268847ed3bf2008e74bce334e810/f/ruby.spec#_187-190
https://src.fedoraproject.org/rpms/ruby/blob/308b2c0ab2c6268847ed3bf2008e74bce334e810/f/ruby.spec#_203-206
https://src.fedoraproject.org/rpms/ruby/blob/308b2c0ab2c6268847ed3bf2008e74bce334e810/f/ruby.spec#_795-800
https://src.fedoraproject.org/rpms/ruby/blob/308b2c0ab2c6268847ed3bf2008e74bce334e810/f/ruby.spec#_1330-1335

Saying all these, it makes me realize that in theory, if the rubygems-devel package was installed during Ruby build, the generators could actually run twice. But they are executed for specific path based on specific macro:

https://src.fedoraproject.org/rpms/ruby/blob/308b2c0ab2c6268847ed3bf2008e74bce334e810/f/rubygems.attr#_6

Not sure. In reality, having Ruby installed during build of Ruby is problematic for different reasons, so I don't have to be worried in this case.

For that you need to make sure that it is only run once during the build - even if the package is actually installed in the system building the package.

Do I? I don't think there is currently anything, which would prevent one generator running twice. E.g. if there are multiple .attr files pointing to the same script. Also the macros can be changed any time to point to the same script. And the generators can also be disabled by changing the macro.

Actually, from this POV, my original PR (making the local_generator special) kind of helps to mitigate the concerns. The more flexibility will provide more opportunities for clashes.

Nevertheless, let me be clear that I am not really arguing for or against of any of those variant. Both versions are acceptable. But it is good to understand all the corner cases 👍

ffesti · 2024-01-18T10:38:48Z

I think the issues is reproducibility and correctness. If you fix the dependency generator in your package you don't want to old - possibly broken - deps still in your package, just because the old package is still on your build system.

voxik · 2024-01-18T11:49:19Z

I think the issues is reproducibility and correctness. If you fix the dependency generator in your package you don't want to old - possibly broken - deps still in your package, just because the old package is still on your build system.

That is actually good point.

So in the Ruby case, it would actually make sense to override the rubygems macros, which would make sure the generator shipped with Ruby is the only one used. IOW there would need to be:

%global __local_file_attrs rubygems

%global __rubygems_requires make -C %{_builddir}/%{buildsubdir}/%{_vpath_builddir} -s runruby TESTRUN_SCRIPT="--enable-gems %{SOURCE9}"	
%global __rubygems_provides make -C %{_builddir}/%{buildsubdir}/%{_vpath_builddir} -s runruby TESTRUN_SCRIPT="--enable-gems %{SOURCE10}"	
%global __rubygems_conflicts make -C %{_builddir}/%{buildsubdir}/%{_vpath_builddir} -s runruby TESTRUN_SCRIPT="--enable-gems %{SOURCE11}"	
%global __rubygems_path ^%{gem_dir}/specifications/.*\.gemspec$

Actually, this leads me to another silly idea. Maybe the rubygems.attr could be loaded instead of the overrides if it was crafted properly ...

ffesti · 2024-01-18T14:51:02Z

OK, just to write down how the current system works:

The fileattrs/*.attr are read in at the beginning with all other macro files. The macros in there can be over written by the spec file - or by %load ing the .attr file in the Sources.

RPM goes through the list of file attrs (currently from globbing the fileattr directory) and stuffs the macros it can find into it's own data structure before evaluating them and running the dependency generators.

The has the disadvantage that when e.g. a _provides script is declared in the installed .attr file it will be excuted unless the macro is #undefined in the spec file. That's kinda unfortunate but something packagers probably can deal with. This also means that if the same name is used in %__packaged_file_attrs the over written macro values will also overwrite the installed settings as the macros are in the same name space. So the old scripts would not be used - only the new ones executed twice. But we can simply filter out the duplicates. Having the file attribute twice doesn't ever make sense as there is only one setting for it.

ffesti · 2024-01-22T13:21:10Z

OK, I replaced the word "local" as its meaning is just too vague here. "Packaged" discourages using file attributes that are just on the machine without being shipped or being installed from another package. While this is technically possible this is something we clearly don't want to encourage.

These patches still need to be squashed and a combined commit message be added. I'll leave them separate for now to make it easier to review the last few changes on their own.

voxik · 2024-01-22T16:36:08Z

docs/manual/dependency_generators.md

+Normally file attributes and their dependency generators are shipped in separate packages that need to be installed before the package making use of them can be build.
+
+Since rpm 4.20 the names of file attribute shipped with the package can be put into the *__packaged_file_attrs* macro separated by colons (:). The macros that normally go into the *\*.attr* files still need to be defined (the dependency generators typically pointing to some Source files or some files in the install root).
+


I think that example would be useful here. Also the following paragraph is not very tangible without example.

voxik · 2024-01-22T16:40:38Z

packaged or package? I am asking, because the generators are not packaged yet as I perceive that. But I can live also with packaged :)

pmatilai · 2024-01-23T08:15:52Z

I find packaged quite misleading, because the case is to allow for attributes and generators that are local to the build. Those attributes may also be packaged, but that's not relevant for this feature. I see two separate main cases for this:

build a package with the generator it ships to avoid having to build twice
build a package with a generator that is only relevant for that package

Back to bike-shedding. __build_file_attrs is closer maybe but then it suggests those are the only attrs for the build. __extra_file_attrs would seem accurate because no matter where they come from they are extra to the regular attrs.

voxik · 2024-01-23T10:02:06Z

__override_file_attrs, which would give higher prominence to the regular installed attrs and discouraged use of this feature. But that brings me back to the __extra_file_attrs, which I have read after I was thinking about __override_file_attrs ;) IOW __extra_file_attrs sounds good to me.

__additional_file_attrs could also be option. It sounds mundane, but maybe that makes it the better choice.

ffesti · 2024-01-31T11:34:01Z

I think the issue here that they can be both overriding existing ones or being an add-on. This makes it difficult to find a good name that covers both cases. OK, technically the macro doesn't override existing file attrs. Overriding/defining the macros does. So here we are registering file attr names...

This all makes __local_file_attrs look less horrible than I first thought __additional_file_attrs is actually kinda close but not quite as it implies that it does not contains file attrs that are already installed. What about __add_file_attrs? It's shorter and does not emphasis as much the separation betwenn new and already existing.

voxik · 2024-01-31T11:42:17Z

__embedded_file_attrs?

pmatilai · 2024-02-02T10:47:23Z

This all makes __local_file_attrs look less horrible than I first thought

Indeed. It's sufficiently vague to cover both cases we care about, and totally accurate in the sense that its local to the build.

__additional_file_attrs is actually kinda close but not quite as it implies that it does not contains file attrs that are already installed. What about __add_file_attrs? It's shorter and does not emphasis as much the separation betwenn new and already existing.

I can live with either local or add. There are bigger crisis in the world 😆

ffesti · 2024-02-06T10:31:52Z

Ok, renamed back to __local_file_attrs. I squashed the patches and improved the docs a little bit. From my POV this is now complete. Can someone please prove read the docs? Thanks!

pmatilai · 2024-02-06T11:48:06Z

build/rpmfc.c

+	// skip file attrs found in __local_file_attrs for now
+	for (j=0; (j < nlocals) && strcmp(bn, local_attrs[j]); j++);
+	if (j < nlocals) {
+	    free(bn);


This is not right, basename() doesn't allocate, so this would free some random pointer inside an allocation.

Might be more readable to just toss all basenames into a new argv with argvAddUniq() , which frees us from having to care about these details here and allows doing rpmfcAttrNew()'s from a single loop.

pmatilai · 2024-02-06T11:48:19Z

build/rpmfc.c

+    for (int i = 0; i < nfiles; i++) {
+	char *bn = basename(files[i]);
+	bn[strlen(bn)-strlen(".attr")] = '\0';
+	// skip file attrs found in __local_file_attrs for now


/* */ comments, thanks.

pmatilai · 2024-02-06T12:17:01Z

tests/rpmbuild.at

+runroot rpmbuild -bb --quiet \
+		--define '__local_file_attrs my_test_attr' \
+		--define '__my_test_attr_provides() foo(%{basename:%{1}})' \
+		--define '__my_test_attr_path .*' \


This covers on of the use-cases, but the excess free() in the code kinda proves that we need a test for the other scenario too where there's a pre-existing attr by the same name.

pmatilai · 2024-02-06T12:19:19Z

One more thing wrt the macro name: I wonder if this is a case where it should not have those leading underscores. The generator itself is full of double underscore names, but the newly added macro here is something directly intended for packager use in a spec. I dunno, it may even be more confusing to have one of the macros without them 😅

voxik · 2024-02-06T12:58:23Z

tests/rpmbuild.at

+RPMDB_INIT
+
+runroot rpmbuild -bb --quiet \
+		--define '__local_file_attrs my_test_attr' \


If this covered multiple generators to demonstrate that something like this is supported, that would be even better

ffesti · 2024-02-13T10:36:41Z

One more thing wrt the macro name: I wonder if this is a case where it should not have those leading underscores. The generator itself is full of double underscore names, but the newly added macro here is something directly intended for packager use in a spec. I dunno, it may even be more confusing to have one of the macros without them 😅

May be someone(tm) should write down the policy regarding underscores. May be in a RPM Design Philosophy document...
IIRC the idea was that macros without underscore are the packagers. One underscore are RPM's macro to be used by packagers and double underscore are internal macros to be left untouched. Not that this really is done that way.

But I agree that there should probably be less underscores.

ffesti · 2024-02-13T12:53:48Z

OK, removed one underscore from the macro name, rewrite the init code and added two test cases that deal with already installed file attributes.

pmatilai · 2024-02-13T13:57:26Z

build/rpmfc.c

+    for (int i = 0; i < nfiles; i++) {
+	char *bn = basename(files[i]);
+	bn[strlen(bn)-strlen(".attr")] = '\0';
+	argvAddUniq(&all_attrs, bn);


The files coming out of rpmGlob() are unique already, you don't need argvAddUniq() here. What I meant is that you could simply use the argv coming from rpmGlob() (the .attr is already getting stripped out) and argvAddUniq() the local stuff to that.

Yes, but they are file names and not just attribute names. An while they are unique because we hard coded the glob, making the location configurable may turn up the same names from different locations. That's why I made the code a bit more defensive than it needs to be. Also I don't even want to think what rpmGlob returns if it doesn't find anything. Overall I don't think this is going to get that much simpler in the end.

Doh, I was somehow thinking rpmGlob() returned basenames there but, but (of course) it doesn't. So scratch that part, sorry.

The location is configurable but it's still always just a single directory, it cannot possibly have multiple files by the same name. It's not that argvAddUniq() is wrong, it just gives the reader the wrong impression basically. Similarly moving this .attr splitting loop out of the rpmGlob() if-block seems strange, because there's only ever anything to do on rpmGlob() success. So that's kinda backwards to what you say about not wanting to think about the case where it doesn't find anything because that's already handled, and just obfuscates the diff for no good reason. It's always better to let the actual change stand out in the diff when reasonably possible.

This is the actual change to the first part here. nattr gets recalculated later on, but most of the time it's already correct from here:

/* Discover known attributes from pathnames + initialize them */ if (rpmGlob(attrPath, NULL, &files) == 0) { nattrs = argvCount(files); - fc->atypes = xcalloc(nattrs + 1, sizeof(*fc->atypes)); for (int i = 0; i < nattrs; i++) { char *bn = basename(files[i]); bn[strlen(bn)-strlen(".attr")] = '\0'; - fc->atypes[i] = rpmfcAttrNew(bn); + argvAdd(&all_attrs, bn); } - fc->atypes[nattrs] = NULL; argvFree(files); } + + /* Get file attributes from _local_file_attrs macro */ + /* ... /

(actually one could just pass &nattrs to rpmGlob() and avoid the argvCount(), but that too would kinda obfuscate the diff)

Patch cleanliness aside, this all needs to be just one commit. It's a single new feature with its tests, we are not interested in PR review in the commit history.

Sure. There also is a "squash and merge" button somewhere down there...

I realised the really right way to do this is to refactor the discovery/init split to a separate commit, and then the local_attr thing doesn't need to change anything at all, only add itself.
Having asked this to be one commit already I couldn't very well ask you to do this, so here goes: #2911

This can declare file attributes which details can be defined in the spec file. This allows enabling file attributes and their dependency generators even if they are only shipped in the package itself and are not yet installed. The names need to be separated by colons (:). Co-authored-by: Florian Festi <[email protected]> Resolves: rpm-software-management#782

ffesti · 2024-02-15T08:02:14Z

Merged as #2911
Thanks for the patch!

voxik force-pushed the local_generator branch 3 times, most recently from 8a532b2 to 61bd40a Compare October 25, 2023 09:08

voxik commented Oct 25, 2023

View reviewed changes

ffesti self-assigned this Jan 17, 2024

ffesti added RFE DONT DO NOT merge, for whatever reason labels Jan 17, 2024

voxik commented Jan 18, 2024

View reviewed changes

ffesti removed the DONT DO NOT merge, for whatever reason label Jan 22, 2024

voxik commented Jan 22, 2024

View reviewed changes

ffesti force-pushed the local_generator branch from 57334a2 to 5ff3074 Compare February 6, 2024 10:29

pmatilai reviewed Feb 6, 2024

View reviewed changes

voxik commented Feb 6, 2024

View reviewed changes

ffesti force-pushed the local_generator branch 3 times, most recently from ab3a293 to 7c64e73 Compare February 13, 2024 12:08

pmatilai reviewed Feb 13, 2024

View reviewed changes

ffesti force-pushed the local_generator branch from e1dea8e to 95b89b6 Compare February 14, 2024 08:33

ffesti force-pushed the local_generator branch from 95b89b6 to 805d831 Compare February 14, 2024 08:38

pmatilai self-assigned this Feb 14, 2024

ffesti closed this Feb 15, 2024

		Normally file attributes and their dependency generators are shipped in separate packages that need to be installed before the package making use of them can be build.

		Since rpm 4.20 the names of file attribute shipped with the package can be put into the __packaged_file_attrs macro separated by colons (:). The macros that normally go into the \.attr* files still need to be defined (the dependency generators typically pointing to some Source files or some files in the install root).

Add "local_generator" #2734

Add "local_generator" #2734

Conversation

voxik commented Oct 24, 2023

pmatilai commented Oct 24, 2023

voxik commented Oct 24, 2023

voxik commented Oct 24, 2023

pmatilai commented Oct 25, 2023

pmatilai commented Oct 25, 2023

voxik commented Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

mizdebsk commented Oct 25, 2023

pmatilai commented Oct 26, 2023

voxik commented Oct 27, 2023

ffesti commented Jan 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voxik commented Jan 18, 2024

voxik commented Jan 18, 2024

ffesti commented Jan 18, 2024

voxik commented Jan 18, 2024 • edited Loading

ffesti commented Jan 18, 2024

voxik commented Jan 18, 2024

ffesti commented Jan 18, 2024

ffesti commented Jan 22, 2024

Choose a reason for hiding this comment

voxik commented Jan 22, 2024

pmatilai commented Jan 23, 2024

voxik commented Jan 23, 2024

ffesti commented Jan 31, 2024

voxik commented Jan 31, 2024

pmatilai commented Feb 2, 2024

ffesti commented Feb 6, 2024

pmatilai Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

pmatilai Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmatilai commented Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

ffesti commented Feb 13, 2024

ffesti commented Feb 13, 2024

pmatilai Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffesti commented Feb 15, 2024

voxik commented Oct 25, 2023 •

edited

Loading

voxik commented Jan 18, 2024 •

edited

Loading

pmatilai Feb 6, 2024 •

edited

Loading

pmatilai Feb 6, 2024 •

edited

Loading

pmatilai commented Feb 6, 2024 •

edited

Loading

pmatilai Feb 13, 2024 •

edited

Loading