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

[OpenFX] New Port #37670

Merged
merged 12 commits into from
Apr 11, 2024
Merged

[OpenFX] New Port #37670

merged 12 commits into from
Apr 11, 2024

Conversation

ramajd
Copy link
Contributor

@ramajd ramajd commented Mar 24, 2024

  • Changes comply with the maintainer guide.
  • The name of the port matches an existing name for this component on https://repology.org/ if possible, and/or is strongly associated with that component on search engines.
  • Optional dependencies are resolved in exactly one way. For example, if the component is built with CMake, all find_package calls are REQUIRED, are satisfied by vcpkg.json's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx.
  • The versioning scheme in vcpkg.json matches what upstream says.
  • The license declaration in vcpkg.json matches what upstream says.
  • The installed as the "copyright" file matches what upstream says.
  • The source code of the component installed comes from an authoritative source.
  • The generated "usage text" is accurate. See adding-usage for context.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is in the new port's versions file.
  • Only one version is added to each modified port's versions file.

@ramajd
Copy link
Contributor Author

ramajd commented Mar 24, 2024

since the previous pull request for OpenFX library (#36927) didn't have any progress since last month, I submitted a new one by myself.

in my opinion the current PR is ready to be merged. however, if any further changes need to be applied, please let me know.

@ramajd
Copy link
Contributor Author

ramajd commented Mar 24, 2024

@microsoft-github-policy-service agree

@WangWeiLin-MV WangWeiLin-MV added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Mar 25, 2024
@ramajd
Copy link
Contributor Author

ramajd commented Mar 27, 2024

@BillyONeal I just extracted the CMake files from the patch to separated files copied using the file(COPY_FILE ...)

ports/openfx/Openfx.cmake Outdated Show resolved Hide resolved
ports/openfx/Openfx.cmake Outdated Show resolved Hide resolved
@ramajd
Copy link
Contributor Author

ramajd commented Mar 28, 2024

@WangWeiLin-MV I just marked the CMake target as unofficial . the usage document is also added to the port

@BillyONeal
Copy link
Member

@BillyONeal I just extracted the CMake files from the patch to separated files copied using the file(COPY_FILE ...)

Thanks! If you think it's ready to go please push the
Ready for review
button :)

@ramajd ramajd marked this pull request as ready for review March 28, 2024 23:34
ports/openfx/Openfx.cmake Outdated Show resolved Hide resolved
ports/openfx/Support.cmake Outdated Show resolved Hide resolved
ports/openfx/portfile.cmake Outdated Show resolved Hide resolved
ports/openfx/Openfx.cmake Outdated Show resolved Hide resolved
ports/openfx/Openfx.cmake Outdated Show resolved Hide resolved
ports/openfx/usage Outdated Show resolved Hide resolved
ramajd added a commit to ramajd/vcpkg that referenced this pull request Mar 29, 2024
@ramajd
Copy link
Contributor Author

ramajd commented Mar 29, 2024

@WangWeiLin-MV I applied all the changes except OFX_SUPPORT_PLUGINS_HEADERS_DIR which is not related to the main SDK header file and only used in examples.

Copy link
Contributor

@WangWeiLin-MV WangWeiLin-MV left a comment

Choose a reason for hiding this comment

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

Since it is a new port, could you please change files End of Line to LF? Thanks in advance.

ports/openfx/Support.cmake Outdated Show resolved Hide resolved
@dg0yt
Copy link
Contributor

dg0yt commented Apr 3, 2024

the version 1.4 is currently used in host applications like Davinci Resolve. so at least we need that version available for now. we can add the latest releases with CMake support included afterwards.

So the CMake project must be reviewed...
I wonder what is the purpose of having two targets, OpenFx and OfxSupport. Does it mirror HEAD's build system?

@ramin-raeisi
Copy link

the version 1.4 is currently used in host applications like Davinci Resolve. so at least we need that version available for now. we can add the latest releases with CMake support included afterwards.

So the CMake project must be reviewed... I wonder what is the purpose of having two targets, OpenFx and OfxSupport. Does it mirror HEAD's build system?

@dg0yt @WangWeiLin-MV

OfxSupport composed of C++ wrappers for OpenFx as, OpenFx is C-based.

ports/openfx/usage Outdated Show resolved Hide resolved
ports/openfx/portfile.cmake Show resolved Hide resolved
ports/openfx/portfile.cmake Outdated Show resolved Hide resolved
ports/openfx/portfile.cmake Outdated Show resolved Hide resolved
WangWeiLin-MV
WangWeiLin-MV previously approved these changes Apr 9, 2024
Copy link
Contributor

@WangWeiLin-MV WangWeiLin-MV left a comment

Choose a reason for hiding this comment

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

Port usage tests pass with following triplets:

  • x64-linux
  • x64-windows
  • x64-windows-static
  • x64-windows-static-md
  • x86-windows

@WangWeiLin-MV WangWeiLin-MV added the info:reviewed Pull Request changes follow basic guidelines label Apr 9, 2024
Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

Community feedback.

ports/openfx/Openfx.cmake Outdated Show resolved Hide resolved
ports/openfx/Openfx.cmake Outdated Show resolved Hide resolved
$<INSTALL_INTERFACE:include/openfx>
)

add_subdirectory(Support)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to merge the contests of the other short file, and then rename this file to CMakeLists.txt.

Comment on lines 5 to 7
set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

Copy link
Contributor

@dg0yt dg0yt Apr 9, 2024

Choose a reason for hiding this comment

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

I think that this is mostly obsolete, given the target_compile_feature below.

Suggested change
set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dg0yt since OpenFX support library can't be built using the C++17, we should stick to C++14, at least for this old version. so I just updated the CMAKE_CXX_STANDARD to 14.

ports/openfx/Openfx.cmake Outdated Show resolved Hide resolved
ports/openfx/portfile.cmake Show resolved Hide resolved
ports/openfx/usage Outdated Show resolved Hide resolved
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

I'm happy with this provided the issues raised by @dg0yt are addressed/considered

@ramajd
Copy link
Contributor Author

ramajd commented Apr 10, 2024

@dg0yt @BillyONeal I just applied the changes related to the provided feedbacks.

@ramajd ramajd requested review from BillyONeal and dg0yt April 10, 2024 10:36
@JavierMatosD
Copy link
Contributor

Please follow @dg0yt's suggestion here. I'll put the PR in draft for now. When the PR is ready please mark as ready for review.

@JavierMatosD JavierMatosD marked this pull request as draft April 10, 2024 17:06
@ramajd
Copy link
Contributor Author

ramajd commented Apr 10, 2024

@JavierMatosD I applied the mentioned changes, now the PR can be reviewed again

@ramajd ramajd marked this pull request as ready for review April 10, 2024 22:12
@WangWeiLin-MV WangWeiLin-MV removed the info:reviewed Pull Request changes follow basic guidelines label Apr 11, 2024
@JavierMatosD JavierMatosD merged commit 8a12a36 into microsoft:master Apr 11, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants