search

pupnp / pupnp

libupnp: Build UPnP-compliant control points, devices, and bridges on several operating systems.
https://pupnp.github.io/pupnp
BSD 3-Clause "New" or "Revised" License
352 stars 117 forks source link

Add LFS flags to pkg-config? #89

Closed jcowgill closed 2 years ago

jcowgill commented 5 years ago

This bug was reported against the Debian package: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=913541#16

The main issue is Debian specific, but one of the suggestions to fixing it was adding LFS flags to the pkg-config file so that reverse-dependencies are built with LFS automatically. I'm torn on this issue because while it is useful, it might "surprise" rdeps who might not realize they are getting built in LFS mode, and if they're poorly programmed they might break.

I'm filing this here in the hope of getting other people's opinions on this :)

ausil commented 5 years ago

I am currently hitting the same bug in Fedora trying to make sure we have largefile support in libpunp. trying to resolve https://github.com/gerbera/gerbera/issues/375

whyman commented 5 years ago

I dont think this would be a nice plan.

There is code to make a build fail https://github.com/mrjimenez/pupnp/blob/162d0a8bd94070446cf02977c7140c51482de256/upnp/inc/FileInfo.h#L21-L23

But that only works is the library is built with LFS support, if it is not, you can still buld your client app with LFS but using a non LFS libupnp.

Perhaps it just needs to be more explicitly a match check.

Any thoughts @ukleinek?

ausil commented 5 years ago

The check should be both ways, additionaly when building libupnp the building of the samples has to use the same options as the library, currently that is not true.

ukleinek commented 5 years ago

The check should be both ways,

Not sure what you mean here. libupnp is supposed to be compiled with LFS (if available). Making this configurable isn't a sensible option in my eyes. (E.g. it makes binaries compiled on a host with a non-LFS libupnp break if executed on a host that has LFS enabled.)

additionaly when building libupnp the building of the samples has to use the same options as the library, currently that is not true.

Without knowing the details: I'd expect the samples to fail to compile then which is a bug. Please report this in a separate issue.

ausil commented 5 years ago

The check should be both ways,

Not sure what you mean here. libupnp is supposed to be compiled with LFS (if available). Making this configurable isn't a sensible option in my eyes. (E.g. it makes binaries compiled on a host with a non-LFS libupnp break if executed on a host that has LFS enabled.)

@ukleinek There is a fundamental issue by having the one library being able to be built with or without largefile support. I his issues with gerbera because libupnp was build without laregfile support and gerbera was built with largefile support, the result was it does not work and is not caught at build time like it would be if the library was built with largefile support and the client tried to build without. thats is what i mean by checking both ways, ensuring consistency.

additionaly when building libupnp the building of the samples has to use the same options as the library, currently that is not true.

Without knowing the details: I'd expect the samples to fail to compile then which is a bug. Please report this in a separate issue. I will file a separate bug for it.

The idea presented in this issue is a way to ensure that the client will always use what the library it is being built against uses, it does not cover the case if you build against one library and run against a completely different one, but very little will unless you have two different library names.

ukleinek commented 5 years ago

The check should be both ways,

Not sure what you mean here. libupnp is supposed to be compiled with LFS (if available). Making this configurable isn't a sensible option in my eyes. (E.g. it makes binaries compiled on a host with a non-LFS libupnp break if executed on a host that has LFS enabled.)

@ukleinek There is a fundamental issue by having the one library being able to be built with or without largefile support. I his issues with gerbera because libupnp was build without laregfile support and gerbera was built with largefile support, the result was it does not work and is not caught at build time like it would be if the library was built with largefile support and the client tried to build without. thats is what i mean by checking both ways, ensuring consistency.

AFAICT libupnp needs patching (or you need to use a rather old version) to compile it without LFS.

Without knowing the details: I'd expect the samples to fail to compile then which is a bug. Please report this in a separate issue. I will file a separate bug for it.

The idea presented in this issue is a way to ensure that the client will always use what the library it is being built against uses, it does not cover the case if you build against one library and run against a completely different one, but very little will unless you have two different library names.

This stops to work once you want to link against two libs. One uses LFS and the other doesn't.

ausil commented 5 years ago

all that is needed is adding --disable-largefile to configure, which is what fedora did to work around #94 which led to the issues I had with gerbera.

What are the ways that libupnp are supposed to work and be used? right now on 32 bit arches you need to either use --disable-largefile or --disable-samples to get a build, each potentially having different unintended side effects. I would also ask how you are supposed to detect when building against libupnp how are you supposed to make sure that you have the correct options enabled? I feel that the best way to make sure you have things right is for libupnp to tell you what you have to use the work with it.

ukleinek commented 5 years ago

Ah, I was not aware that --disable-largefile exists. Hmm, I think we should better not support that.

Vollstrecker commented 2 years ago

As it's in the config, and was explcitely added in cmake also, and it's in the headers, this can be closed. @jcowgill As it seems from this bugreport that you are the maintainer in debian, maybe you should consider an update, 1.8.4 is old even for Debian.