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

libvpx: support for conan v2 #13799

Merged
merged 28 commits into from
Jan 27, 2023
Merged

Conversation

paulharris
Copy link
Contributor

Started work, got this weird message and I'm wondering if conan is injecting --bindir

Any advice please?

libvpx/1.11.0: Calling:
 > "/build/conandata/conan-data/libvpx/1.11.0/_/_/build/a7d7b9773babc7a104bc397939b61bb2babf96bc/src/configure" '--disable-shared' '--enable-static' '--prefix=/' '--bindir=${prefix}/bin' '--sbindir=${prefix}/bin' '--libdir=${prefix}/lib' '--includedir=${prefix}/include' '--oldincludedir=${prefix}/include' '--disable-examples' '--disable-unit-tests' '--disable-tools' '--disable-docs' '--enable-vp9-highbitdepth' '--as=yasm' '--target=x86_64-linux-gcc' '--disable-avx' '--disable-avx2' '--disable-avx512' 
  disabling shared
Unknown option "--bindir=${prefix}/bin".

@conan-center-bot

This comment has been minimized.

@paulharris
Copy link
Contributor Author

I thought I should document this somewhere...
perhaps there is a better way to solve this problem?

libvpx's configure seems to do strange things. Strange to me anyway.

On Linux, libvpx's "configure" doesn't understand --bindir and --includedir, so I "fixed it" by adding:

def layout(self):
        basic_layout(self, src_folder="src")
        self.cpp.package.bindirs = []
        self.cpp.package.includedirs = []

and, it doesn't seem to substitute ${prefix}

This results in:

libvpx/1.11.0: Calling:
 > "/build/conandata/conan-data/libvpx/1.11.0/_/_/build/a7d7b9773babc7a104bc397939b61bb2babf96bc/src/configure" '--disable-shared' '--enable-static' '--prefix=/' '--libdir=${prefix}/lib' '--disable-examples' '--disable-unit-tests' '--disable-tools' '--disable-docs' '--enable-vp9-highbitdepth' '--as=yasm' '--target=x86_64-linux-gcc' '--disable-avx' '--disable-avx2' '--disable-avx512' 
...snip...
Libdir ${prefix}/lib must be a subdirectory of 
...snip...
	ConanException: Error 1 while executing "/build/conandata/conan-data/libvpx/1.11.0/_/_/build/a7d7b9773babc7a104bc397939b61bb2babf96bc/src/configure" '--disable-shared' '--enable-static' '--prefix=/' '--libdir=${prefix}/lib' '--disable-examples' '--disable-unit-tests' '--disable-tools' '--disable-docs' '--enable-vp9-highbitdepth' '--as=yasm' '--target=x86_64-linux-gcc' '--disable-avx' '--disable-avx2' '--disable-avx512' 

So I add self.cpp.package.libdirs = []
and then I add:

        tc.configure_args.extend([
            "--prefix=/output",
            "--libdir=/output/lib",

It then builds and puts the output files into:
/build/conandata/conan-data/libvpx/1.11.0/_/_/package/a7d7b9773babc7a104bc397939b61bb2babf96bc/output/include/vpx/vp8.h
Note the extra output.
(I originally tried self.package_folder, and the result was /packagefolder/packagefolder/lib)

It would be idea if that wasn't added, but if I add what is effectively the defaults:

        tc.configure_args.extend([
            "--prefix=/",
            "--libdir=/lib",

Then I get this complaint from configure:
Libdir /lib must be a subdirectory of
Which is a result of this bit of code in libvpx's configure:

post_process_common_cmdline() {
  prefix="${prefix:-/usr/local}"
  prefix="${prefix%/}"
  libdir="${libdir:-${prefix}/lib}"
  libdir="${libdir%/}"
  if [ "${libdir#${prefix}}" = "${libdir}" ]; then
    die "Libdir ${libdir} must be a subdirectory of ${prefix}"
  fi
}

A simple / prefix and /lib is apparently not enough to satisfy this test.

So my end "solution" is:

def generate(self):
        tc.configure_args.extend([
            "--prefix=/output_goes_here",
            "--libdir=/output_goes_here/lib",
...
def package(self):
        rename(self,
                os.path.join(self.package_folder, "output_goes_here", "include"),
                os.path.join(self.package_folder, "include")
                )
        rename(self,
                os.path.join(self.package_folder, "output_goes_here", "lib"),
                os.path.join(self.package_folder, "lib")
                )
        rmdir(self, os.path.join(self.package_folder, "output_goes_here"))

I literally used "output_goes_here" for easy grepping.

@paulharris
Copy link
Contributor Author

note that the MSVC conversion hasn't begun yet ...

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@paulharris
Copy link
Contributor Author

Recipe is getting close, would like to start getting some reviews please.
Especially on the whole LTO / -GL flag detection in CFLAGS, that is all above my head.
I wonder how to enable LTO anyway ?

@conan-center-bot

This comment has been minimized.

@paulharris
Copy link
Contributor Author

@prince-chrismc linter doesn't like the line:
from conan.tools.microsoft.visual import msvc_version_to_vs_ide_version
https://github.com/conan-io/conan-center-index/actions/runs/3404784548/jobs/5662271149
Error: recipes/libvpx/all/conanfile.py:9: [E9011(conan-import-tools), ] Import tools following pattern 'from conan.tools.xxxx import yyyyy' (https://docs.conan.io/en/latest/reference/conanfile/tools.html).

@paulharris paulharris marked this pull request as ready for review November 6, 2022 14:45
MartinDelille
MartinDelille previously approved these changes Dec 22, 2022
Copy link
Contributor

@MartinDelille MartinDelille left a comment

Choose a reason for hiding this comment

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

I'm just a community reviewer and it looks good to me but let's see what CCI maintainers have to say about it!

self.copy("vpx*.lib", src=libdir, dst="lib")

autotools = Autotools(self)
# NOT USED # autotools.configure() # FIXME can use this if we can remove --host and --build flags
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice job by the way! I had a bit of struggle trying to make a conan v2 version of Ffmpeg and your work may be very helpful!

prince-chrismc
prince-chrismc previously approved these changes Dec 27, 2022
Copy link
Contributor

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

There's a lot of work here, I did not see any blockers so i'd suggest leaving any smaller issue for the next PR :)

@conan-center-bot

This comment has been minimized.

@prince-chrismc
Copy link
Contributor

The linter check is required but blocked since its a private import

@MartinDelille
Copy link
Contributor

If I understand correctly, this recipe could be use AutotoolsToolchain once we will be using conan 1.57.0, right ?

@paulharris
Copy link
Contributor Author

If I understand correctly, this recipe could be use AutotoolsToolchain once we will be using conan 1.57.0, right ?

Do you mean, for building for Windows? I'm not sure, it would be nice to just use one build method...

@paulharris
Copy link
Contributor Author

The linter check is required but blocked since its a private import

So, how to progress this? Duplicate the logic?

@SSE4
Copy link
Contributor

SSE4 commented Jan 16, 2023

The linter check is required but blocked since its a private import

So, how to progress this? Duplicate the logic?

create an issue in conan repo to expose msvc_version_to_vs_ide_version. if it's needed for recipes, it's needed. duplicating logic everywhere is not an option.
for the time being, I think linter can be suppressed temporarily (@prince-chrismc there were some sort of no-lint directive?)

@SSE4
Copy link
Contributor

SSE4 commented Jan 16, 2023

AutotoolsToolchain

AFAIK libvpx is not autotools-based. it uses its own scripts, "similar" to autotools, but written from scratch. it understands some common arguments and variables, but not all of them, unfortunately.

@prince-chrismc
Copy link
Contributor

I raised the issue to the team but missed the last sync so I am not sure the status. I'll ping them again if there's no response

@paulharris
Copy link
Contributor Author

AutotoolsToolchain

AFAIK libvpx is not autotools-based. it uses its own scripts, "similar" to autotools, but written from scratch. it understands some common arguments and variables, but not all of them, unfortunately.

This is correct. So we have to use their MS Build stuff for Windows.

@prince-chrismc
Copy link
Contributor

added to weekly meeting for the team to review :)

@prince-chrismc prince-chrismc dismissed stale reviews from MartinDelille and themself via 85e96ca January 26, 2023 19:46
@paulharris
Copy link
Contributor Author

Thanks @prince-chrismc for following up with this, have been busy elsewhere so I'm very happy to see progress!

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 46 (85e96caafbf82a6a69fa80981ff93cc7aa634899):

  • libvpx/1.10.0@:
    All packages built successfully! (All logs)

  • libvpx/1.9.0@:
    All packages built successfully! (All logs)

  • libvpx/1.11.0@:
    All packages built successfully! (All logs)

@prince-chrismc
Copy link
Contributor

I totally feel you, super glad to get this over the finish

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

Tricky recipe, thank you for detailing all questions!

@conan-center-bot conan-center-bot merged commit 395ea9a into conan-io:master Jan 27, 2023
@paulharris paulharris deleted the libvpx-conan-v2 branch February 1, 2023 02:51
StellaSmith pushed a commit to StellaSmith/conan-center-index that referenced this pull request Feb 2, 2023
* libvpx: started work on conan v2

* Workaround for libvpx's unusual configure

* Minor fix

* Fixes for building on Windows

* Fixes for package(), related to 'output_here' ./configure workaround

* Clean-up imports

* Use f'' strings

* Try leaving CFLAGS/CXXFLAGS untouched

* Can't use tools_legacy as an import alias anymore

* Typo

* Link to m and pthread, for Linux and FreeBSD

* Fixup tools.microsoft.bash:path usage

* Fix libvpx configure script, don't assume gcc can be used as the linker

* Added notes on libvpx recipe debugging, not easy.

* Enable recipe debugging

* Call configure with our own custom command

* Support shared builds

* unix_path the configure script path

* Upgrade away from get_env(), and fix for debug build on windows

* Fix debug build on Linux

* Fix for windows

* Disable recipe debugging

* Re-enable recipe debugging, lets see what it does

* Disable recipe debugging, the build works

* Upgrade for conan 1.54, use stdcpp_library

from conan.tools.build import stdcpp_library

* Minor cleanup

* Apply suggestions from code review

Co-authored-by: Martin Delille <martin@delille.org>

* dont use private import

Co-authored-by: Martin Delille <martin@delille.org>
Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
# look for "output_goes_here" below for more in this thread.
self.cpp.package.bindirs = []
self.cpp.package.includedirs = []
self.cpp.package.libdirs = [] # not strictly necessary, but lets do all as a group
Copy link
Contributor

Choose a reason for hiding this comment

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

this trick defeats fix_apple_shared_install_name() and even the hook checking whether shared libs are relocatables.

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 might be able to use tc.update_configure_args({"--bindir":None}) instead (or something like that).
I won't be able to look for another ~12 hours, if you have the time please feel free to fix up and suggest more :)

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.

8 participants