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

feat(build): include cpp guideline support library #4776

Closed
wants to merge 1 commit into from

Conversation

Swiftb0y
Copy link
Member

@github-actions github-actions bot added the build label May 30, 2022
# Cpp Core Guideline Support Library (GSL)
include(FetchContent)

FetchContent_Declare(GSL
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this work without internet access during the build? If yes then in which directory are those downloaded archives expected to reside in?

Please compare with how libkeyfinder is integrated. This allows to manually download the archives in advance and run the build without an internet connection.

Otherwise it would break at least the RPM Fusion builds.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mhmmm yeah, this was the recommend integration by MS, I'll look into offline ways to build this.

@daschuer
Copy link
Member

Our traditional way of integration would be just to copy it to our lib folder. It is only a set of header files anyway. And has the advantage to be part of the tar ball.
If we decide for the just in time download, every package maintainer needs to keep it working on their build environment.
I think in case of Ubuntu we need to maintain an extra deb package which I am not funt of. We do this in case of libdjinterop.

@daschuer
Copy link
Member

https://github.com/gsl-lite/gsl-lite maintains a single header version. Interesting in our case.
Will increase build time though ..

@daschuer
Copy link
Member

Ah, there is already a Linux package:
https://repology.org/project/ms-gsl/versions

ms-gsl is also part of VCPKG
https://github.com/Microsoft/vcpkg/tree/master/ports/ms-gsl

So much guess we should use that. I can take care.

@Swiftb0y
Copy link
Member Author

Unfortunately I have zero idea how vcpkg works (or cmake for that matter) so I'd really appreciate if someone other than me could take over here.

@JoergAtGithub
Copy link
Member

Ah, there is already a Linux package:
https://repology.org/project/ms-gsl/versions

The package on Linux package on Ubuntu Focal seems rather old. I think it would make sense to use the same version on all platforms here. The single header gsl-lite sounds like the best fitting solution to me.

@daschuer
Copy link
Member

The package on Linux package on Ubuntu Focal seems rather old.

Was there a change to gsl::not_null that we really need since 2.1.0? I guess it will work good enough anyway.
Using the distro version releases use he question how to get the files into the tar ball.

But both solutions will work for me.

@Swiftb0y
Copy link
Member Author

I don't want to rely on the distribution for a header-only library. Also this only helps us on Linux. IMO using vcpkg or manually downloading it like with libkeyfinder makes more sense.

@daschuer
Copy link
Member

daschuer commented Jun 1, 2022

VCPKG + relying on distros comes in a union.
VCPKG is our the "distro" for MacOs and windows.

I would prefer either to literally copy the required files into our lib folder, or rely on the distros, because both produces the minimum hassles.

The normal pattern we use until now is to rely on the.distros and overlay them only if required by our lib folder.

Not sure if this also applies here, because it is "only" a header file.

In addition I see on the horizon two patches, a specialisation for 'std::unique_ptr' and a replacement of "Expects" implemented by assert() by our own asserts,

What do you think?

@daschuer
Copy link
Member

daschuer commented Jun 1, 2022

The vcpkg install PR is here: mixxxdj/vcpkg#44
Unfortunately building with Mixxx is currently broken in #4717 due to an upstream issue waiting here for merge: microsoft/vcpkg#25018. If this will not be merged in a short time we may consider to merge this PR to our repro to not being blocked.

@Swiftb0y
Copy link
Member Author

This has been superseeded by #4782
Thank you @daschuer

@Swiftb0y Swiftb0y closed this Jun 13, 2022
@Swiftb0y Swiftb0y deleted the gsl branch June 13, 2022 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants