Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
Merged as #2911 Thanks for the patch! -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1945539722 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
Closed #2734. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#event-11811695188 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@pmatilai commented on this pull request. > 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); - } - fc->atypes[nattrs] = NULL; - argvFree(files); + nfiles = argvCount(files); +} +for (int i = 0; i < nfiles; i++) { + char *bn = basename(files[i]); + bn[strlen(bn)-strlen(".attr")] = '\0'; + argvAddUniq(&all_attrs, bn); 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: https://github.com/rpm-software-management/rpm/pull/2911 -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#discussion_r1490545272 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@ffesti pushed 1 commit. 805d83186bec6297ef35a1b0d1405aec845c069a Add %_local_file_attrs macro -- View it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734/files/95b89b63ab47a576e1006e512cb14935d980361e..805d83186bec6297ef35a1b0d1405aec845c069a You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@ffesti commented on this pull request. > 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); - } - fc->atypes[nattrs] = NULL; - argvFree(files); + nfiles = argvCount(files); +} +for (int i = 0; i < nfiles; i++) { + char *bn = basename(files[i]); + bn[strlen(bn)-strlen(".attr")] = '\0'; + argvAddUniq(&all_attrs, bn); Sure. There also is a "squash and merge" button somewhere down there... -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#discussion_r1489096578 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@pmatilai commented on this pull request. > 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); - } - fc->atypes[nattrs] = NULL; - argvFree(files); + nfiles = argvCount(files); +} +for (int i = 0; i < nfiles; i++) { + char *bn = basename(files[i]); + bn[strlen(bn)-strlen(".attr")] = '\0'; + argvAddUniq(&all_attrs, bn); 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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#discussion_r1489081084 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@ffesti commented on this pull request. > 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); - } - fc->atypes[nattrs] = NULL; - argvFree(files); + nfiles = argvCount(files); +} +for (int i = 0; i < nfiles; i++) { + char *bn = basename(files[i]); + bn[strlen(bn)-strlen(".attr")] = '\0'; + argvAddUniq(&all_attrs, bn); 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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#discussion_r1488176745 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@pmatilai commented on this pull request. > 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); - } - fc->atypes[nattrs] = NULL; - argvFree(files); + nfiles = argvCount(files); +} +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() and argvAddUniq() the local stuff to that. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#pullrequestreview-1878019777 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
OK, removed one underscore from the macro name, rewrite the init code and added two test cases that deal with already installed file attributes. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1941456054 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@ffesti pushed 1 commit. e1dea8eafaf3f4da91e7ba132f9e953eec3a9665 Add more test cases -- View it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734/files/7c64e73308a328d3aad40c118024f799503bc96f..e1dea8eafaf3f4da91e7ba132f9e953eec3a9665 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@ffesti pushed 1 commit. 7c64e73308a328d3aad40c118024f799503bc96f Local File Attrs: Remove one underscore -- View it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734/files/ab3a293498ec59129d3551e76e444e964b9f0985..7c64e73308a328d3aad40c118024f799503bc96f You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@ffesti pushed 1 commit. 7023620eb258af338b1e53b3806b8f66ad9384d7 Local File Attrs: Remove one underscore -- View it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734/files/fb9b6ec25e2e72daa7944175e64c2928104cd016..7023620eb258af338b1e53b3806b8f66ad9384d7 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@ffesti pushed 1 commit. fb9b6ec25e2e72daa7944175e64c2928104cd016 Local File Attrs: Remove one underscore -- View it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734/files/5ff3074187b888f9ff62416d9495fe36f7890468..fb9b6ec25e2e72daa7944175e64c2928104cd016 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
> 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](#2897) 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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1941125050 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@voxik commented on this pull request. > @@ -995,6 +995,25 @@ runroot rpm -qp --requires > /build/RPMS/noarch/shebang-0.1-1.noarch.rpm|grep -v ^ []) RPMTEST_CLEANUP +AT_SETUP([Local dependency generator]) +AT_KEYWORDS([build]) +RPMTEST_CHECK([ +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 -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#pullrequestreview-1865093933 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
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 those, but the newly added macro here is something directly intended for packager use. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1929405020 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@pmatilai commented on this pull request. > @@ -995,6 +995,25 @@ runroot rpm -qp --requires > /build/RPMS/noarch/shebang-0.1-1.noarch.rpm|grep -v ^ []) RPMTEST_CLEANUP +AT_SETUP([Local dependency generator]) +AT_KEYWORDS([build]) +RPMTEST_CHECK([ +RPMDB_INIT + +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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#pullrequestreview-1865004852 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@pmatilai commented on this pull request. > - bn[strlen(bn)-strlen(".attr")] = '\0'; - fc->atypes[i] = rpmfcAttrNew(bn); + nfiles = argvCount(files); +} +char * local_attr_names = rpmExpand("%{?__local_file_attrs}", NULL); +ARGV_t local_attrs = argvSplitString(local_attr_names, ":", ARGV_SKIPEMPTY); +nlocals = argvCount(local_attrs); +fc->atypes = xcalloc(nfiles + nlocals + 1, sizeof(*fc->atypes)); + +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 + for (j=0; (j < nlocals) && strcmp(bn, local_attrs[j]); j++); + if (j < nlocals) { + free(bn); 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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#discussion_r1479661477 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@pmatilai commented on this pull request. > - 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); + nfiles = argvCount(files); +} +char * local_attr_names = rpmExpand("%{?__local_file_attrs}", NULL); +ARGV_t local_attrs = argvSplitString(local_attr_names, ":", ARGV_SKIPEMPTY); +nlocals = argvCount(local_attrs); +fc->atypes = xcalloc(nfiles + nlocals + 1, sizeof(*fc->atypes)); + +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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#pullrequestreview-1864948185 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@pmatilai commented on this pull request. > - bn[strlen(bn)-strlen(".attr")] = '\0'; - fc->atypes[i] = rpmfcAttrNew(bn); + nfiles = argvCount(files); +} +char * local_attr_names = rpmExpand("%{?__local_file_attrs}", NULL); +ARGV_t local_attrs = argvSplitString(local_attr_names, ":", ARGV_SKIPEMPTY); +nlocals = argvCount(local_attrs); +fc->atypes = xcalloc(nfiles + nlocals + 1, sizeof(*fc->atypes)); + +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 + 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 the one that is. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#pullrequestreview-1864947811 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
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! -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1929219489 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@ffesti pushed 1 commit. 5ff3074187b888f9ff62416d9495fe36f7890468 Add %__local_file_attrs macro -- View it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734/files/57334a2b0b0ad7d84e8e398bf6c6e6a8b53d2481..5ff3074187b888f9ff62416d9495fe36f7890468 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
> 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 :laughing: -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1923549189 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
`__embedded_file_attrs`? -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1918940894 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1918928307 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
`__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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1905700754 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1905503860 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
`packaged` or `package`? I am asking, because the generators are not packaged yet as I perceive that. But I can live also with `packaged` :) -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1904388907 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@voxik commented on this pull request. > @@ -132,6 +132,14 @@ Enabling the multifile mode is done by setting %__foo_protocol multifile ``` +## Using File Attributes in their own Package + +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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#pullrequestreview-1836891196 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1903994707 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@ffesti pushed 2 commits. 2468b76d5b7e4bae8c55ddabb188d0d2f4807dbf Filter duplicate file attributes 57334a2b0b0ad7d84e8e398bf6c6e6a8b53d2481 Add documentation for __packaged_file_attrs -- View it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734/files/5059816af9185cc7c3f19ad729b4dc657f912cde..57334a2b0b0ad7d84e8e398bf6c6e6a8b53d2481 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1898627307 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@ffesti pushed 1 commit. 5059816af9185cc7c3f19ad729b4dc657f912cde Rename __local_file_attrs to __packaged_file_attrs -- View it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734/files/cd4bd81ad1c446c119a27eec77b2828b5ea1624a..5059816af9185cc7c3f19ad729b4dc657f912cde You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
> 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 ... -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1898330857 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1898223470 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
> 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 👍 -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1898123753 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1898072680 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
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). -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1898067418 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@voxik commented on this pull request. > @@ -866,6 +866,7 @@ RPMTEST_CHECK([ RPMDB_INIT runroot rpmbuild -bb --quiet \ + --define '__local_file_attrs local_generator' \ Maybe something like `totally_random_generator_name` -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#discussion_r1457125066 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
> 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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1898047160 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@voxik commented on this pull request. > @@ -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 -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#pullrequestreview-1829143312 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1895983933 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@ffesti pushed 1 commit. cd4bd81ad1c446c119a27eec77b2828b5ea1624a Add %__local_file_attrs macro -- View it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734/files/61bd40a9df5170da6182e560d172fb16f4e3213b..cd4bd81ad1c446c119a27eec77b2828b5ea1624a You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
> 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 https://github.com/rpm-software-management/rpm/issues/782#issuecomment-1747200612 and put this idea into practice for [Fedora](https://src.fedoraproject.org/rpms/rpm-local-generator-support). 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). -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1782567947 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1780526290 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
If `local_generator.attr` file exists then `local_generator` created twice. Why not simply create an empty `local_generator.attr` file instead? -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1779258988 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@voxik commented on this pull request. > 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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#pullrequestreview-1696861838 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
> 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 added also some 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 -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1778907220 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@voxik pushed 1 commit. 61bd40a9df5170da6182e560d172fb16f4e3213b Add "local_generator" -- View it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734/files/8a532b24e527f48cc45f7f49fc24d6fc4be39d49..61bd40a9df5170da6182e560d172fb16f4e3213b You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@voxik pushed 1 commit. 8a532b24e527f48cc45f7f49fc24d6fc4be39d49 Add "local_generator" -- View it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734/files/33c10c89387b168bceaa93ee2be7c6210a90aa2e..8a532b24e527f48cc45f7f49fc24d6fc4be39d49 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
@voxik pushed 1 commit. 33c10c89387b168bceaa93ee2be7c6210a90aa2e Add "local_generator" -- View it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734/files/4b04cc38167dd637c3c1f68bf6d858453ccf24a1..33c10c89387b168bceaa93ee2be7c6210a90aa2e You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
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? -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1778645654 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1778627591 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
> > 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 ... -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1777647182 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
> 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 :( -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1777641462 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)
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) :sweat_smile: -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2734#issuecomment-1777463960 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint