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:
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
@pmatilai commented on this pull request.
> if (rpmGlob(attrPath, NULL, ) == 0) {
- nattrs = argvCount(files);
- fc->atypes = xcalloc(nattrs + 1, sizeof(*fc->atypes));
- for (int i = 0; i < nattrs; i++) {
- char *bn = basename(files[i]);
-
@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
@ffesti commented on this pull request.
> if (rpmGlob(attrPath, NULL, ) == 0) {
- nattrs = argvCount(files);
- fc->atypes = xcalloc(nattrs + 1, sizeof(*fc->atypes));
- for (int i = 0; i < nattrs; i++) {
- char *bn = basename(files[i]);
-
@pmatilai commented on this pull request.
> if (rpmGlob(attrPath, NULL, ) == 0) {
- nattrs = argvCount(files);
- fc->atypes = xcalloc(nattrs + 1, sizeof(*fc->atypes));
- for (int i = 0; i < nattrs; i++) {
- char *bn = basename(files[i]);
-
@ffesti commented on this pull request.
> if (rpmGlob(attrPath, NULL, ) == 0) {
- nattrs = argvCount(files);
- fc->atypes = xcalloc(nattrs + 1, sizeof(*fc->atypes));
- for (int i = 0; i < nattrs; i++) {
- char *bn = basename(files[i]);
-
@pmatilai commented on this pull request.
> if (rpmGlob(attrPath, NULL, ) == 0) {
- nattrs = argvCount(files);
- fc->atypes = xcalloc(nattrs + 1, sizeof(*fc->atypes));
- for (int i = 0; i < nattrs; i++) {
- char *bn = basename(files[i]);
-
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
@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
@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
@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
@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
> 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
@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 \
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:
@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
@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 =
@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);
+
@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 =
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:
@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
> 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
>
`__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
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
`__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
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
`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
@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
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
@ffesti pushed 2 commits.
2468b76d5b7e4bae8c55ddabb188d0d2f4807dbf Filter duplicate file attributes
57334a2b0b0ad7d84e8e398bf6c6e6a8b53d2481 Add documentation for
__packaged_file_attrs
--
View it on GitHub:
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
@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
> 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
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:
> 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
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
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
@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
> 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`
@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
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
@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
> 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
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:
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
@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
> 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
@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
@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
@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
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
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
> > 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
> 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
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
56 matches
Mail list logo
