Set some dependency options to be multi-choice. #1758
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vrabaud
force-pushed
the
cmake_cleanup
branch
from
54e5b2b to
d9dd2ee
Compare
vrabaud
changed the title
|
@fdintino , @wantehchang , what do you think? BTW, this is the official way of doing things: https://www.kitware.com//constraining-values-with-comboboxes-in-cmake-cmake-gui/ |
|
I like it. I've also seen projects accept paths for these options, to force find_package to look in a specific prefix. |
vrabaud
force-pushed
the
cmake_cleanup
branch
from
d9dd2ee to
6e20d9b
Compare
wantehchang
approved these changes
Nov 17, 2023
|
Vincent: In addition to the changelog, please also update https://github.com/AOMediaCodec/libavif/blob/main/README.md. I think this is a good simplification of the cmake options. Thanks. |
vrabaud
force-pushed
the
cmake_cleanup
branch
2 times, most recently
from
c07124b to
fe444ff
Compare
vrabaud
changed the title
fdintino
reviewed
Nov 20, 2023
The current options are complex: - you want a local AOM, you need to set to variables (AVIF_CODEC_AOM and AVIF_LOCAL_AOM to ON) - they can be confusing: some work in best effort, e.g. if you do not AVIF_LOCAL_LIBYUV, it tries to find libyuv if not, oh well, too bad let's move on. - you cannot disable some options: if libyuv is found, use it! Oh wait, I actually never asked for it (causing issues like AOMediaCodec#1732) That's why I propose to set dependency options to: OFF: it will never be used whether it is findable or in ext LOCAL: it has to use the version in ext/ and will fail if not found SYSTEM: it will find_package it and fail if not found An AUTO version (that uses a dependency if found) should not be added as it will create more confusion for developers or package building.
Add backward compatibility
Update readme and changelog. Fix CMake comment.
vrabaud
force-pushed
the
cmake_cleanup
branch
from
fe444ff to
6ac2459
Compare
vrabaud
force-pushed
the
cmake_cleanup
branch
from
6ac2459 to
ed14e34
Compare
wantehchang
reviewed
Nov 21, 2023
| @@ -51,8 +52,9 @@ list of requirements. | |||
|
|
|||
| If you want to statically link any codec into your local (static) build of | |||
| libavif, building using one of these scripts and then enabling the associated | |||
| `AVIF_LOCAL_*` is a convenient method, but you must make sure to disable | |||
| `BUILD_SHARED_LIBS` in CMake to instruct it to make a static libavif library. | |||
| `AVIF_LOCAL_*` and setting `AVIF_CODEC_*` to `LOCAL` is a convenient method, | |||
There was a problem hiding this comment.
The new text mentions both the old and new ways, connected by "and". I think we just need to mention the new way ("setting AVIF_CODEC_* to LOCAL"). If we mention both, they should be connected by "or" because one of them is sufficient.
There was a problem hiding this comment.
Thx, fixed in #1795 I believe
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current options are complex:
That's why I propose to set dependency options to: OFF: it will never be used whether it is findable or in ext LOCAL: it has to use the version in ext/ and will fail if not found SYSTEM: it will find_package it and fail if not found
An AUTO version (that uses a dependency if found) should not be added as it will create more confusion for developers or package building.
This is compatible with
ccmakeorcmake-gui.