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

Add recipe for the bmx library provided by the BBC #23955

Merged
merged 21 commits into from
Jun 19, 2024

Conversation

irieger
Copy link
Contributor

@irieger irieger commented May 10, 2024

Specify library name and version: bmx/1.2 and bmx/cci.20240510

Currently I'm implementing something using the bmx library for handling video IO. Obviously, as I'm really settled on Conan now, I first created a package for the library which is so far missing.

As I'm looking into some options just recently added, besides the most recent 1.2 release I also added a snapshot in the usual form of cci.DATE for the current master branch.


@AbrilRBS AbrilRBS self-assigned this May 10, 2024
Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

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

Thanks! I'll have to check with the team about our handling of the -O2 directives present in the upstream cmakelist, but other than that and the few comments I've left, this looks great so far, thanks! :)

recipes/bmx/all/conanfile.py Outdated Show resolved Hide resolved
recipes/bmx/all/conanfile.py Show resolved Hide resolved
@conan-center-bot conan-center-bot added Failed Missing dependencies Build failed due missing dependencies in Conan Center labels May 10, 2024
@conan-center-bot

This comment has been minimized.

@irieger
Copy link
Contributor Author

irieger commented May 10, 2024

Any idea why uriparser isn't found in Conan 1?

ERROR: Missing prebuilt package for 'uriparser/0.9.8'

https://c3i.jfrog.io/c3i/misc/logs/pr/23955/3-linux-gcc-legacy/bmx/1.2//e2b8caa5d5af82d1dc00027a5bac8fb9e82e8e46-build.txt

@AbrilRBS
Copy link
Member

I'll regenerate the missing binaries now and restart the PR once they are ready

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@irieger
Copy link
Contributor Author

irieger commented May 11, 2024

@RubenRBS Again as in some other PRs, the result of the Conan 2 CI step doesn't get published here so I can't see the errors. At least locally I had working builds on Linux, Mac and Windows already so I'd like to see what fails.

@AbrilRBS
Copy link
Member

@irieger sorry about that. Sometimes the bot is a bit too eager to post th results and ends up not waiting for the v2 pipeline if it takes too long to finish - We're moving away from this solution in the new piepeline we're currently implementing, so that will be less of an issue soon-ish, thanks for your patience meanwhile!

The errors taht would have been posted are here: https://c3i.jfrog.io/c3i/misc-v2/summary.html?json=https://c3i.jfrog.io/c3i/misc-v2/logs/pr/23955/5-windows-msvc/bmx/1.2//summary.json

Nevertheless I've now restarted the CI, the missing binaries were built a few days back but I haven't had a chance to trigger the CI again, let's see if that fixes the v1 piepeline completely :)

@conan-center-bot

This comment has been minimized.

@AbrilRBS
Copy link
Member

The fact the uriparser is still missing tells us that there' something going on with some of the dependecies, will check on Monday, thanks for your patience :)

@conan-center-bot

This comment has been minimized.

@irieger
Copy link
Contributor Author

irieger commented May 12, 2024

Somehow it tries to link to MXF.lib instead of MXF.dll when building shared. I'm trying to understand why that is but so far I haven't found any hint. The directory to the target is correct and the dll is there. Any idea where such a thing could come from?
I'm not really experienced with Windows development and its behaviour and on both macOS and Linux it also correctly solves to either the .a or .so/.dylib.

@conan-center-bot

This comment has been minimized.

@irieger
Copy link
Contributor Author

irieger commented May 13, 2024

@RubenRBS Regarding your comment about -O2 in the first comment: Do you think it makes sense to get a patch upstream to remove a fixed -O2 in the CMake file? How is that normally done in Conan builds? Just checked my toolchain and found no hint where it might include any optimization options for the Release, unless they are hidden somewhere in one of the variables that are propagate, but search shows no "-O2". (Haven't anything manually in my profile.)

The BMX project seems very open to pull requests and was very helpful so far. So I assume if we suggest changes that don't affect the non-conan build cases, they are open to include stuff.

@irieger
Copy link
Contributor Author

irieger commented May 17, 2024

Patched out the -O2 flag instances in the 1.2 release and bumped the CCI version to include the latest changes on master integrating all the fixes (fixed CXX version, -O2 flag and C++20 compatibility code fixes). That reduces the needed CMake patching to a minimal modification on how the dependencies are searched and linked.

@conan-center-bot

This comment has been minimized.

@conan-center-bot conan-center-bot removed the Missing dependencies Build failed due missing dependencies in Conan Center label May 22, 2024
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@AbrilRBS
Copy link
Member

Hi @irieger thanks for your patience - I tried to reproduce the windows issue but I found an issue where the dependency graph contains strawberryperl which does not support non x86 archs, I'll try with a different system, or pinging @uilianries who I know has a x86_64 windows :)

@AbrilRBS
Copy link
Member

Update, I've been able to reproduce this locally on my personal machine, will try to debug this tomorrow :)

@irieger
Copy link
Contributor Author

irieger commented May 29, 2024

Hi @irieger thanks for your patience - I tried to reproduce the windows issue but I found an issue where the dependency graph contains strawberryperl which does not support non x86 archs, I'll try with a different system, or pinging @uilianries who I know has a x86_64 windows :)

No worries. I have days where I'm a bit nervous but since I setup my local artifactory a few months ago and run it with my own branch of CCI to allow some small modifications etc., I also occasionally pull in some stuff that isn't merged in upstream yet and can easily use it still.

@conan-center-bot

This comment has been minimized.

@irieger
Copy link
Contributor Author

irieger commented Jun 15, 2024

I digged a bit deeper and think I found why building as a shared library on Windows fails. As I learned now, it seems that Windows creates a lib file also for shared libraries, unlike Linux or macOS where you either get an a or a dylib/so. As this project doesn't define any exports in either the files nor via the Cmake flag to export all symbols on Windows.

Locally I tried it with a patch adding set_target_properties(MXF PROPERTIES WINDOWS_EXPORT_ALL_SYMBOLS TRUE) and equivalent. While it doesn't fail with the error of not finding the MXF.lib then, it fails to find some symbol. I'll add an issue tomorrow on the bmx project github. But from looking at things now after some reading I think it might have just not been tried on Windows as a shared lib so far.

So I'd suggest - if this works out in the CI now - that we merge it with a validation to prevent windows shared builds. And if it gets fixed in the upstream I'll integrate the fix later. If we agree on that I'd add a descriptive comment for possible users in the validate function with a link to the issue after I create it tomorrow.

@conan-center-bot

This comment has been minimized.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 17 (97e76c5042fa48fa5933a93ac9979fa86d40bd06):

  • bmx/1.2:
    All packages built successfully! (All logs)

  • bmx/cci.20240517:
    All packages built successfully! (All logs)


Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 17 (97e76c5042fa48fa5933a93ac9979fa86d40bd06):

  • bmx/cci.20240517:
    All packages built successfully! (All logs)

  • bmx/1.2:
    All packages built successfully! (All logs)

Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

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

Thanks! Let's go with this for now until we figure out what's going on in Windows, sorry we couldn't find the issue either in our end :)

@conan-center-bot conan-center-bot merged commit e0571e4 into conan-io:master Jun 19, 2024
18 checks passed
@irieger irieger deleted the bmx/add-recipe branch June 19, 2024 10:22
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.

None yet

5 participants