Skip to content

Conversation

@y-guyon
Copy link
Contributor

@y-guyon y-guyon commented Feb 27, 2025

No description provided.

@y-guyon
Copy link
Contributor Author

y-guyon commented Feb 27, 2025

@wantehchang wantehchang requested a review from jzern February 27, 2025 13:54
@wantehchang
Copy link
Collaborator

Yannis: Thanks a lot for writing this pull request. I asked James to review this.

I suggest we follow this OS support table: https://github.com/google/oss-policies-info/blob/main/foundational-cxx-support-matrix.md

@y-guyon
Copy link
Contributor Author

y-guyon commented Feb 27, 2025

I do not fully grasp the implications of passing this flag or not and using a newer or older version.

Could you describe what could go wrong in these scenarios:

  • Binary compiled with CMAKE_OSX_DEPLOYMENT_TARGET=X and run on platform version < X (the OS just refuses to run that binary, right?)
  • Binary compiled with CMAKE_OSX_DEPLOYMENT_TARGET=X and run on platform version >= X (all is fine)
  • Binary compiled without CMAKE_OSX_DEPLOYMENT_TARGET (I guess CMake assigns some value X depending on the environment at compile time and this scenario is equivalent to the two others)

I understood that setting a target version makes sure no system call introduced in a later platform version is used. If that is the only impact, why not use the oldest possible target version?
From what I saw, setting CMAKE_OSX_DEPLOYMENT_TARGET does not change the SDK version used to compile libavif anyway.

Same questions as above for CMAKE_VS_WINDOWS_TARGET_PLATFORM_MIN_VERSION instead of CMAKE_OSX_DEPLOYMENT_TARGET if the behavior is different on Windows.


Should we use CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION, CMAKE_VS_WINDOWS_TARGET_PLATFORM_MIN_VERSION, or both?

@jzern
Copy link
Contributor

jzern commented Feb 27, 2025

I do not fully grasp the implications of passing this flag or not and using a newer or older version.

Could you describe what could go wrong in these scenarios:

  • Binary compiled with CMAKE_OSX_DEPLOYMENT_TARGET=X and run on platform version < X (the OS just refuses to run that binary, right?)

This sets -mmacosx-min-version / -mmacos-min-version, so in general that would be my expectation, unless no new functions were used and you got lucky in the shared object dependencies.

  • Binary compiled with CMAKE_OSX_DEPLOYMENT_TARGET=X and run on platform version >= X (all is fine)
  • Binary compiled without CMAKE_OSX_DEPLOYMENT_TARGET (I guess CMake assigns some value X depending on the environment at compile time and this scenario is equivalent to the two others)

I understood that setting a target version makes sure no system call introduced in a later platform version is used. If that is the only impact, why not use the oldest possible target version?

Security / bug fixes in the library functions would be the main reason. But if the goal is to support back to some version it's what you're left with.

From what I saw, setting CMAKE_OSX_DEPLOYMENT_TARGET does not change the SDK version used to compile libavif anyway.

https://cmake.org/cmake/help/latest/variable/CMAKE_OSX_DEPLOYMENT_TARGET.html. So for the most part just -mmacos-version-min will be set. It sounds like in cases where multiple versions of the SDK were installed this might also influence that.

Same questions as above for CMAKE_VS_WINDOWS_TARGET_PLATFORM_MIN_VERSION instead of CMAKE_OSX_DEPLOYMENT_TARGET if the behavior is different on Windows.

Should we use CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION, CMAKE_VS_WINDOWS_TARGET_PLATFORM_MIN_VERSION, or both?

It sounds like CMAKE_VS_WINDOWS_TARGET_PLATFORM_MIN_VERSION won't be used for non-WindowsStore projects:

https://cmake.org/cmake/help/latest/prop_tgt/VS_WINDOWS_TARGET_PLATFORM_MIN_VERSION.html#prop_tgt:VS_WINDOWS_TARGET_PLATFORM_MIN_VERSION

CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION will choose a SDK version. I guess if you wanted to support the oldest versions of Windows 10, you could specify it, but I feel like this might be something to address if there's a compatibility issue reported by a user.

@y-guyon y-guyon requested a review from jzern February 28, 2025 09:55
@y-guyon
Copy link
Contributor Author

y-guyon commented Feb 28, 2025

It sounds like CMAKE_VS_WINDOWS_TARGET_PLATFORM_MIN_VERSION won't be used for non-WindowsStore projects:

https://cmake.org/cmake/help/latest/prop_tgt/VS_WINDOWS_TARGET_PLATFORM_MIN_VERSION.html#prop_tgt:VS_WINDOWS_TARGET_PLATFORM_MIN_VERSION

It is unclear to me if "the target platform minimum version will not be specified for the project" when:

  • VS_WINDOWS_TARGET_PLATFORM_MIN_VERSION is not specified, or
  • it is not a WindowsStore project.

CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION will choose a SDK version. I guess if you wanted to support the oldest versions of Windows 10, you could specify it, but I feel like this might be something to address if there's a compatibility issue reported by a user.

It is unclear to me what "might be something to address if there's a compatibility issue reported by a user".
Should we NOT use CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION until a user complains, or should we DO use CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION set to a recent version and downgrade it once a user needs it?

@jzern
Copy link
Contributor

jzern commented Feb 28, 2025

It is unclear to me what "might be something to address if there's a compatibility issue reported by a user".
Should we NOT use CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION until a user complains, or should we DO use CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION set to a recent version and downgrade it once a user needs it?

I was thinking we would leave things as is and wait for a report about incompatibility. That might be a little fragile if we move the builder to Windows 11. We can try what you have and see if there are any issues.

jzern
jzern previously approved these changes Feb 28, 2025
Copy link
Contributor

@jzern jzern left a comment

Choose a reason for hiding this comment

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

Actually I wonder if CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION only affects the Visual Studio generator; NInja is being used now.

@jzern jzern dismissed their stale review February 28, 2025 21:21

Actually I wonder if CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION only affects the Visual Studio generator; NInja is being used now.

https://cmake.org/cmake/help/latest/variable/CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION.html

When targeting Windows 10 and above, Visual Studio Generators for VS 2015 and above support specification of a Windows SDK version:

@jzern
Copy link
Contributor

jzern commented Feb 28, 2025

With -G Ninja:

CMake Warning:
  Manually-specified variables were not used by the project:

    CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION

@y-guyon
Copy link
Contributor Author

y-guyon commented Mar 3, 2025

Are you suggesting dropping -G Ninja in general?
I myself often favor omitting the generator selection argument and use cmake --build instead of make or ninja.
Do you know why we select ninja instead of letting CMake pick something?

@jzern
Copy link
Contributor

jzern commented Mar 3, 2025

Are you suggesting dropping -G Ninja in general? I myself often favor omitting the generator selection argument and use cmake --build instead of make or ninja. Do you know why we select ninja instead of letting CMake pick something?

If we want to go forward with the Visual Studio variables we will need to use the Visual Studio project generator. I'm guessing ninja was used because locally it will be faster since it will parallelize on the file level, not project. For something like there's less of a need for it.

@y-guyon y-guyon added this to the v1.2.1 milestone Mar 5, 2025
@y-guyon y-guyon requested a review from jzern March 5, 2025 10:41
@y-guyon y-guyon requested a review from jzern March 6, 2025 12:41
@y-guyon y-guyon merged commit 3e305e7 into AOMediaCodec:main Mar 7, 2025
@y-guyon y-guyon deleted the artifact_versions branch March 7, 2025 08:29
jzern added a commit to jzern/libavif that referenced this pull request Mar 8, 2025
* main: (53 commits)
  Fix reading gray/color ICC profile from color/gray image in apps (AOMediaCodec#2675)
  Revert "Use SVT_LOG=0 in avifsvttest for SVT-AV1 3.0.0 (AOMediaCodec#2668)" (AOMediaCodec#2674)
  Fix monochrome ICC profile error message in apps (AOMediaCodec#2673)
  Add missing changelog for `avifdec --index all` (AOMediaCodec#2672)
  Specify target platform version in binary artifacts (AOMediaCodec#2652)
  avidec: output all frames of animations when passed `--index all` (AOMediaCodec#2670)
  Do not allow keeping the ICC when converting from RGB to gray (AOMediaCodec#2669)
  Update CHANGELOG (AOMediaCodec#2666)
  Use SVT_LOG=0 in avifsvttest for SVT-AV1 3.0.0 (AOMediaCodec#2668)
  read.c: fix empty struct initializers (AOMediaCodec#2661)
  avifutil.c: fix avifQueryCPUCount() empty param list (AOMediaCodec#2662)
  Add libyuv dependency to the README.md (AOMediaCodec#2664)
  fix: patch libyuv CMakeLists for compatibility with gcc 10 (AOMediaCodec#2660)
  Use default for tensorflow with libavm (AOMediaCodec#2624)
  Bump some Rust dependencies (AOMediaCodec#2658)
  Use C99 syntax to initialize avifFileType struct (AOMediaCodec#2657)
  Bump the github-actions group with 3 updates
  libargparse: use a git patch file instead of sed command (AOMediaCodec#2654)
  y4m: Remove some repeated code
  Remove fully static instructions (AOMediaCodec#2649)
  ...
y-guyon added a commit to y-guyon/libavif that referenced this pull request Mar 14, 2025
y-guyon added a commit to y-guyon/libavif that referenced this pull request Mar 14, 2025
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.

3 participants