vasi / squashfuse

FUSE filesystem to mount squashfs archives
Other
288 stars 66 forks source link

Install ALL headers #68

Closed ferdnyc closed 2 years ago

ferdnyc commented 2 years ago

Back in #36 I submitted a PR to fix installation of the header files. At the time, I included only the ones that were #included from squashfuse.h itself, making that header file #include-able.

I figured the other headers that weren't referenced were probably private API that could be omitted.

Well, whether they were intended that way or not, wouldn't you know, at least one project needs them. AppImageKit, which uses sqfs_makedev() from nonstd-makedev.c, will fail on a missing nonstd.h when attempting to build against a squashfuse installed with the current header configuration.

This PR thus updates the config to install every header in the repo.

This includes 'fuseprivate.h' and 'nonstd-internal.h' which may really be private API. I've included them for the sake of completeness, but I can remove either or both if you'd prefer.

probonopd commented 2 years ago

What about /usr/local/include/squashfuse/ll.h?

https://github.com/probonopd/static-tools/pull/17#issuecomment-1114281602

ferdnyc commented 2 years ago

What about /usr/local/include/squashfuse/ll.h?

probonopd/static-tools#17 (comment)

That's added here (next-to-last line), it's inside an if SQ_WANT_LOWLEVEL. I don't know why:

https://github.com/vasi/squashfuse/blob/e51978cd6bb5c4d16fae9eee43d0b258f570bb0f/Makefile.am#L80-L89

Edit: None of the other headers #include it, though, so it should be safe that way. It's not a very useful header without the lowlevel support compiled in, I wouldn't think.

probonopd commented 2 years ago

Is SQ_WANT_LOWLEVEL documented? How does one pass it to the build system?

ferdnyc commented 2 years ago

@probonopd

Looks like it's enabled or disabled, by default, as a result of automake autoconf discovery for SQ_FUSE_API_LOWLEVEL. (Which m4/squashfuse_fuse.m4 determines the value of by checking whether fuse_session_loop is declared in fuse_lowlevel.h.) If it is present, the configure flag --disable-low-level can still turn it off despite the successful detection.

ferdnyc commented 2 years ago

Interestingly, fuseprivate.h will still always do a #include <fuse3/fuse_lowlevel.h>, if the FUSE API version is 30 or greater (FUSE 3.0+). So the effects of --disable-low-level may be limited to switching off internal code paths, without necessarily removing their dependencies.

(Or, maybe it's a sign I really shouldn't include fuseprivate.h with the installed headers. It's another one that none of the other headers include, so it's not a required export.)

vasi commented 2 years ago

Oof, I don't know what I think about this. Could we get AppImageKit to just fix their usage? There's really no reason for them to need sqfs_makedev, why would you have a device inside an AppImage?

probonopd commented 2 years ago

Seems like we need extern dev_t sqfs_makedev(int maj, int min); for this:

https://github.com/probonopd/static-tools/blob/fc1d0f1a970385dac41d115f22f3ee84b3e52757/src/runtime/runtime.c#L380-L382

What would be a better/the correct way to do this?

vasi commented 2 years ago

It looks like static-tools just has a random copy of all of sqfs_stat. But the only thing it actually seems to use is st.st_mode--so there's no point having all of sqfs_stat. Just use inode->base.mode, and then you won't need makedev.

ferdnyc commented 2 years ago

But the only thing it actually seems to use is st.st_mode--so there's no point having all of sqfs_stat. Just use inode->base.mode, and then you won't need makedev.

That looks right to me. private_sqfs_stat(), like sqfs_stat(), just sets st_mode by copying the value of inode->base.mode. The other logic is only there to populate other fields of the struct stat... which you'll never look at, so who needs them?

Given the outcome here, I think I'll close this as unnecessary. In favor of a PR to static-tools that removes private_sqfs_stat() entirely (and the header dependency with it).

vasi commented 2 years ago

Thank you!

ferdnyc commented 2 years ago

(Actually, make that a PR to AppImageKit. static-tools has its own, complicated requirements that also depend on lots of sqfs_ll internals, it's probably always going to need to make copies of the private headers. But that doesn't mean they need to be part of the squashfuse install; static-tools is a specialized consumer.)