Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkgconfig: Generate -uninstalled.pc files #4436

Merged
merged 2 commits into from
Feb 5, 2020

Conversation

xclaesse
Copy link
Member

Closes: #3472.

Copy link
Member

@jon-turney jon-turney left a comment

Choose a reason for hiding this comment

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

Is it possible to write a test case for this?

docs/markdown/Pkgconfig-module.md Outdated Show resolved Hide resolved
docs/markdown/Pkgconfig-module.md Outdated Show resolved Hide resolved
@xclaesse xclaesse force-pushed the pkgconfig-uninstalled branch 2 times, most recently from 3aefc6c to 6dec3a8 Compare October 30, 2018 13:55
@xclaesse
Copy link
Member Author

test case added.

@xclaesse xclaesse force-pushed the pkgconfig-uninstalled branch 2 times, most recently from 79c9f7b to b8b7608 Compare October 30, 2018 18:17
@xclaesse
Copy link
Member Author

I'm not sure to understand the CI failure, with cygwin the library in builddir is named libfoo-1.dll and that doesn't match -lfoo ? It gets renamed on install?

@jon-turney
Copy link
Member

jon-turney commented Oct 30, 2018

I'm not sure to understand the CI failure, with cygwin the library in builddir is named libfoo-1.dll and that doesn't match -lfoo ? It gets renamed on install?

No, that would be bad.

The thing the linker should be looking for is the import library libfoo.dll.a, but it seems that's not being named correctly (it gets called liblibfoo.dll.a). I think that might be a bug with name_prefix : '' on cygwin.

@jon-turney
Copy link
Member

Hmmm.

2d5172b and 071264c makes this test pass for me.

But I'd like to think a bit more about how name_prefix should interact with import library naming.

@jon-turney
Copy link
Member

jon-turney commented Oct 31, 2018

But I'd like to think a bit more about how name_prefix should interact with import library naming.

Naming the import library correctly in this way is only important when we install the library and subsequently link with it using -l, internal uses are fine as we just use the filename.

The relevant PR seems to be #899. The pkgconfig module only handles 2 scenarios: (i) name_prefix is defaulted, or (ii) name_prefix is empty, and the library name starts with lib. Anything else causes a warning, as generating linker flags is problematic.

Anyhow, please squash 071264c as a fix to your new testcase, and add 2e62422 as a fix to an existing bug.

@xclaesse
Copy link
Member Author

xclaesse commented Nov 1, 2018

@jon-turney I included both your patches in my PR. I simplified 071264c to not save/restore env. I think you should still create its own PR for 2e62422 because that looks like an important fix that shouldn't block on getting this -uninstalled pc files feature approved.

@xclaesse xclaesse force-pushed the pkgconfig-uninstalled branch 2 times, most recently from fc3ca93 to 57c4cff Compare November 4, 2018 22:36
@jon-turney
Copy link
Member

I simplified 071264c to not save/restore env.

Thanks.

I think you should still create its own PR for 2e62422 because that looks like an important fix that shouldn't block on getting this -uninstalled pc files feature approved.

Done as #4480

@xclaesse
Copy link
Member Author

@jpakkane @nirbheek Would be great to have a review, I have patches blocked by this for GStreamer.

@jpakkane
Copy link
Member

I really, really don't like the concept of -uninstalled. It gives the indication that we are committing to keeping some sort of stability for things inside the build dir, which we are never going to do. What is the actual use case for this? Who is using it? Could we solve the same problem in some other way instead?

@xclaesse
Copy link
Member Author

We don't need to guarantee any stability here, because if we change the builddir structure, we can always update the -uninstalled generator accordingly.

We have gst-build that is a meson project that builds all gstreamer modules and deps as subprojects. We have a script gst-uninstalled that start a dev env with PKG_CONFIG_PATH, LD_LIBRARY_PATH, etc to be able to quickly build and run an app using the gstreamer version built by meson. That avoids having to run "ninja install" all the time.

So that's really a developer tool, not something distro will use of course.

@xclaesse xclaesse force-pushed the pkgconfig-uninstalled branch 4 times, most recently from 0239662 to 2d30fb4 Compare November 24, 2018 03:48
@textshell
Copy link
Contributor

I think it's important to have a way to use meson build libraries without installing them in many situations.

Installation into a common prefix gets very messy fast, because things are not really independent.

And as soon as a user has many independent leaf projects depending on a forest of libraries that are all unter active development there needs to be a reliable solution to manage that without having to rebuild everything as subprojects for each leaf project.

I've even made a wrapper to allow usage of uninstalled libraries for some of my projects. I don't think forcing installation to a temporary location is a good alternative either, because then you loose installation as the process that places the files in their final location. Often mixing development with uninstalled pieces of code and installation after everything is in a good state without recompiling the whole project is a desirable path.

That said i think we need to come up with a good solution for the include paths in the uninstalled variant. I believe we might have to some some restrictions on possible directory structures. I.e. if the includes are restructured on install i think we should not try to support that. But if a project wants to have a good uninstalled story and places all includes already in suitable directories we should support that.

test cases/common/48 pkgconfig-gen/meson.build Outdated Show resolved Hide resolved
docs/markdown/Pkgconfig-module.md Outdated Show resolved Hide resolved
docs/markdown/snippets/uninstalled-pkgconfig.md Outdated Show resolved Hide resolved
@xclaesse xclaesse force-pushed the pkgconfig-uninstalled branch from 2d30fb4 to e46d308 Compare December 3, 2018 19:09
@xclaesse
Copy link
Member Author

xclaesse commented Dec 3, 2018

Updated this PR.

I changed the directory for uninstalled pc files to <builddir>/meson-uninstalled to use the meson- prefix like we already have for meson-logs and meson-private, and also because we could maybe need to generate other 'uninstalled' helpers stuff some day.

It now implicitly adds -I<source_dir>/<library_subdir> and -I<builddir_dir>/<library_subdir> in Cflags, because it's common to have headers in the same dir as source files, and we already have an implicit include_directories('.') at build time. That should also cover the case we have generated headers.

That said i think we need to come up with a good solution for the include paths in the uninstalled variant. I believe we might have to some some restrictions on possible directory structures. I.e. if the includes are restructured on install i think we should not try to support that. But if a project wants to have a good uninstalled story and places all includes already in suitable directories we should support that.

It is really common to have headers in the same directory as source files, then install them all into the same location. That's what GStreamer does and I'm working on this PR specifically for that project. Of course that means that when using uninstalled pc files the app could include private headers that are not normally installed. But there is nothing we can do against that, I don't think it's a big deal.

@xclaesse xclaesse force-pushed the pkgconfig-uninstalled branch 2 times, most recently from 07bcd58 to 2664284 Compare November 26, 2019 01:48
@xclaesse
Copy link
Member Author

xclaesse commented Dec 1, 2019

@textshell @jpakkane ping? I think this is ready to merge.

@xclaesse
Copy link
Member Author

xclaesse commented Dec 7, 2019

@textshell @jpakkane if nobody object, I'll just go ahead and merge this.

@textshell
Copy link
Contributor

I'm sorry that i can't commit to a timely rereview of this. But from what i have seen nothing substantial changed since the time where i voiced my objections.

I know this is very frustrating. I'm sorry for that. But i don't think your last comment is in the least helpful, as you very well know that the contribution guidelines explicitly require an "ack" from @jpakkane for new features and not just silence after voicing at least major discomfort.

@jpakkane
Copy link
Member

jpakkane commented Dec 7, 2019

I have never particularly liked the uninstalled setup, because it is a maintenance burden. Basically what we are doing here is working around the way Cargo has steadfastly refused to integrate with any other workflow than their own.

@xclaesse
Copy link
Member Author

xclaesse commented Dec 7, 2019

I'm sorry that i can't commit to a timely rereview of this. But from what i have seen nothing substantial changed since the time where i voiced my objections.

While I agree your suggestion is marginally better, the implementation is significantly harder and is a lot more intrusive. I did implement it (see history of this PR) but nobody reviewed it, and it conflicted badly. I decided to revert back to original idea to keep it simple.

I know this is very frustrating. I'm sorry for that. But i don't think your last comment is in the least helpful, as you very well know that the contribution guidelines explicitly require an "ack" from @jpakkane for new features and not just silence after voicing at least major discomfort.

Side note: As the silence on this PR shows, I think @jpakkane as way too much on his plate already, we should learn to delegate more in the Meson project.

I have never particularly liked the uninstalled setup, because it is a maintenance burden.

That's exactly the reason why I reverted this PR to the much less invasive implementation. This patch is pretty well self contained, it does not interfere with the rest of the Meson code base. As for the maintenance burden, I'm totally happy to take responsibility of the pkgconfig module since I perfectly understand you already have too much to work on. If someone report an issue against uninstalled.pc files, just leave it to me ;-)

Basically what we are doing here is working around the way Cargo has steadfastly refused to integrate with any other workflow than their own.

This cargo stuff is just one new type of use-case where I found uninstalled pc files could be useful. But the main use-case remains a developer tool. This has been blocking GStreamer to switch to pkgconfig generator for over a year now because some developers rely on uninstalled pc files in their workflow. https://gitlab.freedesktop.org/gstreamer/gstreamer/merge_requests/4.

@xclaesse
Copy link
Member Author

Ping again, it's sad to miss again the release deadline here. I've got patches waiting on gstreamer blocked by this, and also rust integration that would profit a lot of it. Given how long this has been waiting, I'm really tempted to just merge it and commit myself in fixing any potential regression.

Btw, I'm also offering to become maintainer of the pkgconfig module, given that I wrote a big part of its logic, in the effort to delegate more responsibilities in the meson project.

@sdroege
Copy link
Contributor

sdroege commented Dec 30, 2019

This works great for me, there's just one problem with other software parsing the output of pkg-config: currently you set the whole path to the shared library in the Libs field. Various other software out there seems to assume that it will contain the library as -lfoo instead there. By having -L/path/to/uninstalled/so -lfoo in there instead this would work fine again.

Was there a specific reason to not do that?

@xclaesse
Copy link
Member Author

By "various other software" you mean cargo at least l, right? I'll get a look at that, I wrote it a long time ago.

@sdroege
Copy link
Contributor

sdroege commented Dec 30, 2019

The Rust pkg-config crate parses the output and converts it to something rustc understands (and so that it works for MSVC and other compilers). I saw similar code parsing the -L and -l flags in other places too though.

@nirbheek
Copy link
Member

I would also like to see this feature in Meson, but I can't officially give a +1 because I haven't really been contributing to Meson lately 🙂

When subdir is '/foo/bar' and prefix '/foo' it was returning '/bar',
which is an absolute path. It was then constructing '-L${prefix}//bar'
with bogus double slash.

When subdir is '/fooo/bar' and prefix '/foo' it was returning 'o/bar'.
@xclaesse xclaesse force-pushed the pkgconfig-uninstalled branch from 2664284 to 648253b Compare December 30, 2019 22:38
@xclaesse
Copy link
Member Author

Ok, changed the implementation to use -L -l pairs instead, not sure why I didn't do that from the beginning, all the code was already there.

I also changed to have ${prefix} set to build directory, and ${srcdir} set to source directory, and made all include paths using them instead of repeating long full paths everywhere. That makes the pc file more readable.

@sdroege
Copy link
Contributor

sdroege commented Dec 31, 2019

Thanks, looks good to me :) Just one minor issue

Libs: -L${prefix}/src  -ldav1d

There are 2 spaces between the -L and -l

@xclaesse xclaesse force-pushed the pkgconfig-uninstalled branch from 648253b to 6d55466 Compare December 31, 2019 13:12
@xclaesse
Copy link
Member Author

There are 2 spaces between the -L and -l

That's unrelated, it's like that in regular pc files too. But fixed it now :)

Also updated version to 0.54 since we missed the 0.53 release.

@gdesmott
Copy link

We'd need this feature in order to be able to build Rust GStreamer plugins on our Windows ci. Any chance to have it merged before the next meson release?

@xclaesse xclaesse modified the milestones: 0.53.0, 0.54.0 Jan 31, 2020
@sdroege
Copy link
Contributor

sdroege commented Feb 4, 2020

Any updates here? We'd also need this for gst-uninstalled to work in a useful way (especially on Windows) where other meson-based projects (like GLib) are built as part of the whole thing and are not available via pkg-config unless everything is installed.

@xclaesse xclaesse merged commit 4c5a952 into mesonbuild:master Feb 5, 2020
@xclaesse xclaesse deleted the pkgconfig-uninstalled branch February 5, 2020 15:54
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.

pkgconfig: Add generator for -uninstalled.pc
8 participants