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

Using libuuid to generate uuids #701

Draft
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

nolan-veed
Copy link
Contributor

Why

For #692

What

  • Created target_nouuid and target_libuuid dirs.
    • Moved uuid.cpp to target_nouuid.
    • Created new uuid.cpp in target_libuuid that uses libuuid to create a uuid_t and unparse it into the supplied buffer.
  • Added test.
  • Did other scons changes.

Testing

Added a success test case. Failure test case panics - so can't add one.

@github-actions github-actions bot added the work in progress Pull request is still in progress and changing label Feb 17, 2024
Copy link

🤖 Upon creation, pull request description does not have a link to an issue. If there is a related issue, please add it to the description using any of the supported formats.

@nolan-veed
Copy link
Contributor Author

@gavv
I'm assuming, we'll need to adjust the dockerfiles repo to install libuuid where available? Any suggestions on which envs should have them?

@gavv
Copy link
Member

gavv commented Feb 18, 2024

I'm assuming, we'll need to adjust the dockerfiles repo to install libuuid where available? Any suggestions on which envs should have them?

Yes. I think it can be added to all envs except env-ubuntu:nolibs (which is used to test build when no system dependencies are pre-installed).

I guess it'd be same as https://github.com/roc-streaming/dockerfiles/pull/17/files


Also, we need to add libuuid to build-3rdparty.py: https://github.com/roc-streaming/roc-toolkit/blob/develop/scripts/scons_helpers/build-3rdparty.py

(The task has a link to example PR that adds a dependency there)

build-3rdparty.py will be used when you run scons --build-3rdparty=libuuid or scons --build-3rdparty=all.

On CI, we should use --build-3rdparty=libuuid in cross-compilations steps.

Copy link
Member

@gavv gavv left a comment

Choose a reason for hiding this comment

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

LGTM! Only one small comment.

3rdparty/SConscript Outdated Show resolved Hide resolved
@gavv gavv added the contribution A pull-request by someone else except maintainers label Feb 20, 2024
@nolan-veed
Copy link
Contributor Author

nolan-veed commented Mar 9, 2024

Also, we need to add libuuid to build-3rdparty.py: https://github.com/roc-streaming/roc-toolkit/blob/develop/scripts/scons_helpers/build-3rdparty.py

I didn't understand this. Can you confirm that we should be building libuuid from source? Shouldn't we just be using what the distro has provided? I thought we were just either using the distro's libuuid or providing the ability to disable it.

(Happy to try and build it from source, if needed.)

@gavv
Copy link
Member

gavv commented Apr 9, 2024

Hi, sorry for delay.

I didn't understand this. Can you confirm that we should be building libuuid from source? Shouldn't we just be using what the distro has provided? I thought we were just either using the distro's libuuid or providing the ability to disable it.

Yes, but that's not done by default though.

We have --build-3rdparty option: https://roc-streaming.org/toolkit/docs/building/developer_cookbook.html#building-dependencies

When it's used, internally we call 3rdparty.py script with a dozen of options.

User can tell scons to build any dependency of roc (or all of them).

It is particularly useful in two cases:

  • When a dependency is not widely available, like openfec. Without --build-3rdparty the user would need to build that dependency from sources and provides path to it to roc.

  • When you're cross-compiling. Without --build-3rdparty user would need to setup a proper sysroot, cross-compile all dependencies by hand (or using some tool like crosstool), then build roc.

This option exists just for convenience - this makes building roc from sources much easier for most users.

In case of libuuid, I think it is available in every distro, so the main use case for auto-building it will be using --build-3rdparty=all when cross-compiling, e.g. like described here: https://roc-streaming.org/toolkit/docs/building/user_cookbook.html#linux-cross-compile

@gavv
Copy link
Member

gavv commented May 13, 2024

Hi, I see that it's still WIP, just a few remarks:

  • It's better to use tarball URL from here instead of github.

    Github source tarballs don't have configure, so we have to generate them using autogen, but tarballs from kernel.org are dist-tarballs with pre-generated configure. When it's available, it's always better to use pre-generated one, because autotools are moody and can work differently depending on your local versions of autoconf archive, aclocal, etc. These dist-tarballs with pre-generated configure are actually the intended way to distribute autotools projects.

  • ARM builds are failing because we need to add libuuid to --build-3rdparty in scripts/ci_checks/linux-arm.

    For some reason those scripts list dependencies manually, but I can't see why. I suggest to try using --build-3rdparty=all in those scripts instead.

  • Other scripts are failing because github source tarball weirdly names directories as 1.2.3 instead of project-1.2.3. Tarballs from kernel.org don't have this problem.

  • Please also add --disable-libuuid to first two steps in conditional-build.sh. This way our CI will check that we can build both with and without libuuid.

@nolan-veed
Copy link
Contributor Author

Hi, I see that it's still WIP, just a few remarks:

Thanks for this. I'll pick it up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution A pull-request by someone else except maintainers work in progress Pull request is still in progress and changing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants