Skip to content

Conversation

@arrowd
Copy link
Contributor

@arrowd arrowd commented May 6, 2024

Some Linux distributions as well as FreeBSD OS install 3rd party software into a prefix that differs from "/usr". This change makes asgen work in such cases.

@arrowd
Copy link
Contributor Author

arrowd commented Jun 10, 2024

Can I get a review on this?

@arrowd
Copy link
Contributor Author

arrowd commented Jul 7, 2024

Monthly bump.

@arrowd
Copy link
Contributor Author

arrowd commented Aug 9, 2024

It's been a year in total while I'm waiting for this PR.

value: '',
description: 'Override the directory where gir-to-d searches for GIR files.'
)
option('localbase', type : 'string', value : '/usr', description : 'Prefix under which all 3rd party software is installed')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the name localbase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I borrowed the terminology from FreeBSD where localbase is a variable (and a sysctl) that contains "path to the local software directory". I'm not really sure what "local" means in this context.

Some Linux distributions as well as FreeBSD OS install 3rd party software into
a prefix that differs from "/usr". This change makes asgen work in such cases.
@arrowd
Copy link
Contributor Author

arrowd commented Aug 18, 2025

Rebased onto C++ version. Didn't run-test it yet, but I basically translated the changes from the D version of this PR.

@arrowd
Copy link
Contributor Author

arrowd commented Aug 30, 2025

@ximion We at FreeBSD need this PR to have AppStream working. Please, can we work through it to get it merged? I will be happy to address any raised comments.

@ximion
Copy link
Owner

ximion commented Aug 30, 2025

Weird, I could have sworn I commented on this ages ago - maybe on the bug report?
In short, I don't like this and this will not be merged as a compilation option. Changing fundamental bahevior depending on how asgen was compiled is very unexpected to the user and should not happen - your version will break all other backends in case someone e.g. wants to analyze an Arch repository on a FreeBSD host system.

Making it a runtime option though is far more likely to work, ideally as part of the backend itself (so, the backend decides which path is valid). Doing this in a way that is still performant is a bit harder though (lots of stupid string concatenations in very hot codepaths, so e.g. for the IconHandler, it would make sense to pre-bake the strings).

I was going to have a look at it (and during the C++ translation I thought I could just unconditionally scan /usr/local as well and fix this, until I got to the IconHandler code...), but so far haven't had the time. I could potentially put some of the basic plumbing in place for it next week (the Config/backend decides the prefix logic), and then you could finish it / test it.

@ximion ximion force-pushed the master branch 2 times, most recently from 7d7c6cd to 30d86a8 Compare September 6, 2025 11:52
@arrowd
Copy link
Contributor Author

arrowd commented Oct 11, 2025

I could potentially put some of the basic plumbing in place for it next week (the Config/backend decides the prefix logic), and then you could finish it / test it.

That would be great. Any progress on this front?

@ximion ximion closed this in 4dde7e4 Oct 19, 2025
@ximion
Copy link
Owner

ximion commented Oct 19, 2025

Please test if this change works for you! Your backend will have to override the dataPrefix() method to set a custom prefix, which should then work fine.

Since this is a runtime option, data will be processed the same way on all platforms with the same backend (and how asgen is compiled won't impact this).

@arrowd
Copy link
Contributor Author

arrowd commented Oct 20, 2025

Great, thank you!

Are changes to datainjectpkg.cpp not needed, though? This code also uses hardcoded "/usr" prefix.

@ximion
Copy link
Owner

ximion commented Oct 20, 2025

Yes, but that's for injected data and we can expect users to use the normal prefix here (it's also much easier to document the feature that way).

ximion added a commit that referenced this pull request Oct 20, 2025
This is required because we change AscCompose to *only* look at
backend-prefixed paths, so we need to make sure that the fake package
adheres to whatever it was that the backend has set.

This is not required for icon paths, as we look at those in /usr
unconditionally.

CC: #132
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.

3 participants