Skip to content

Add a top-level --with-system-libs arg to build.sh #42120

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

Merged
merged 1 commit into from
Jul 12, 2024

Conversation

omajid
Copy link
Member

@omajid omajid commented Jul 11, 2024

This lets users build the VMR with a top-level flag like this:

$ /build.sh --source-only --with-system-libs brotli+libunwind+rapidjson+zlib

This was suggested during PR review at:
#41984 (comment)

This lets users build the VMR with a top-level flag like this:

    $ /build.sh --source-only --with-system-libs brotli+libunwind+rapidjson+zlib

This was suggested during PR review at:
dotnet#41984 (comment)
@omajid omajid requested review from a team as code owners July 11, 2024 23:10
@ghost ghost added Area-Infrastructure untriaged Request triage from a team member labels Jul 11, 2024
@omajid
Copy link
Member Author

omajid commented Jul 11, 2024

cc @akoeplinger

@omajid
Copy link
Member Author

omajid commented Jul 11, 2024

Does it also make sense to support the plain argument to default all libs to come from system so the distros don't need to chase adding new ones when we add new dependencies?

I thought about this and have mixed feelings about this. On one hand, like you say, it will help some distros. On the other hand, a number of distros I tried out wont be able to use all. For example. on Fedora, I will not be able to use system libunwind on arm64. On RHEL, I will not be able to use system rapidjson.

Maybe it would be better to document this better?

What about adding top-level flag for each library separately, like this:

./build.sh --with-system-brotli --with-system-libunwind --with-system-rapidjson --with-system-zlib

And these top-level flags would be document via ./build.sh --help.

@ViktorHofer ViktorHofer enabled auto-merge (squash) July 12, 2024 07:30
@akoeplinger
Copy link
Member

What about adding top-level flag for each library separately, like this

That's the same then as listing each lib in --with-system-libs so doesn't provide much benefit and we'd need to touch the script everytime we add a lib. Given an "all" concept doesn't really work given what you said about distro differences I think the current approach makes the most sense.

@ViktorHofer ViktorHofer merged commit 5d2b7f6 into dotnet:main Jul 12, 2024
37 checks passed
@tmds
Copy link
Member

tmds commented Aug 21, 2024

Does it also make sense to support the plain argument to default all libs to come from system so the distros don't need to chase adding new ones when we add new dependencies?

My 2 cents.

I think the default experience should match with what a distro maintainer probably prefers: system libs or bundled libs.

If we think that preference is system libs, it would make sense the argument controls what bundled libs to use instead of what system libs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants