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

[ogre] make zziplib, freeimage, assimp, freetype and less strict resource manager as feature #15194

Merged
merged 1 commit into from Jan 5, 2021

Conversation

dweckmann
Copy link
Contributor

Describe the pull request

  • What does your PR fix?

Some Ogre functionalities are not mandatory and they require additional dependencies which could be problematic, most notably zzip which have a GPL licensing. This PR add features, with default set "as before", so people could easily choose which dependencies they could afford.

Another feature is the ability to use less strict resource manager, so some legacy code would work, while preserving fast lookup for new code.

  • Which triplets are supported/not supported? Have you updated the CI baseline?

All currently supported triplets should still be supported.

@dweckmann dweckmann force-pushed the ogre-limit-dependencies branch 4 times, most recently from f56260a to 28531cb Compare December 18, 2020 14:49
@Hoikas Hoikas mentioned this pull request Dec 19, 2020
@NancyLi1013 NancyLi1013 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Dec 21, 2020
@NancyLi1013
Copy link
Contributor

Have you tested these features?

@dweckmann
Copy link
Contributor Author

dweckmann commented Dec 23, 2020

Have you tested these features?

We don't use them, but, as I said, the previous version of the package did build them. I've just added a way to deactivate them.

@NancyLi1013 NancyLi1013 added requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function and removed requires:author-response labels Dec 25, 2020
@NancyLi1013
Copy link
Contributor

Hi @dweckmann

I tested this features on my local and failed with the following error:

CMake Error: The following variables are used in this project, but they are set to NOTFOUND.
Please set them or make sure they are set and tested correctly in the CMake files:
DirectX9_D3DX9_LIBRARY (ADVANCED)
    linked by target "RenderSystem_Direct3D9" in directory F:/ogre/vcpkg/buildtrees/ogre/src/eddf310f0b-6ab1152694.clean/RenderSystems/Direct3D9

Note: I installed ogre[d3d9,java,python,csharp,overlay,zziplib,strict].

Could you please look into this?

@dweckmann
Copy link
Contributor Author

DirectX9

Well, this option come from the original recipe and the fail seems linked to CMake not finding Directx 9 libraries. Not sure I can do anything, because it is either a cmake issue or platform issue. BTW, I have no access right now to a dev environment with the old directx 9 libraries installed. Maybe should we contact the original author of the port file ?

@dweckmann
Copy link
Contributor Author

Note: I installed ogre[d3d9,java,python,csharp,overlay,zziplib,strict].

Could you please look into this?

Hello again. I can confirm the behavior is the same prior my PR. [d3d9] only works when cmake could find the DirectX9 headers. I guess is it perfectly normal that you encounters the quoted error and this is linked your environment so it is not really an"issue".

What should I do now ?

@NancyLi1013
Copy link
Contributor

Thanks for your further investigation about this issue.

Have you tested other features on your machine? Do they work fine?

@dweckmann
Copy link
Contributor Author

Have you tested other features on your machine? Do they work fine?

As I said, I've really tested that the additional features are not built when not used. Our project build and run correctly when using ogre[core]. I assume, since the optional features where already there before, that the previous contributor have thoroughly tested the now optional features. However, I have at least tested the build of some feature combination [core,assimp,zziplib,assimp], but not the usage because I don't have another project that make use of them. BTW this is what the CI test since the optional features are default features.

Please, tell me if I can do something else.

@NancyLi1013
Copy link
Contributor

Thanks for your response.

Since these new features are set as default features, there is no need to test them now.

I have checked that ogre[d3d9] is broken with the same error on current master branch.

So this PR is good for me.

@NancyLi1013 NancyLi1013 added info:reviewed Pull Request changes follow basic guidelines and removed requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function requires:author-response labels Jan 5, 2021
@dweckmann
Copy link
Contributor Author

I have checked that ogre[d3d9] is broken with the same error on current master branch.

Thanks,

To be clear, ogre[d3d9] is not technically broken, just you must have d3d9 available on your platform, which require some setup.

@vicroms vicroms merged commit 215d38b into microsoft:master Jan 5, 2021
@dweckmann dweckmann deleted the ogre-limit-dependencies branch January 5, 2021 20:20
@traversaro
Copy link
Contributor

I experienced a failure in a downstream CI (see robotology/robotology-superbuild-dependencies-vcpkg#39) due to the fact that this PR enabled by default the strict resource mode, while before by default the strict mode was set to OFF. While I see the use of the strict option, I think that for the time being it could make sense for it to be opt-in by default for backward compatibility, as any user of vcpkg's ogre port until now could be relying on it, while any user interested in strict mode can enable it if necessary. What do you think @dweckmann @vicroms ?

@traversaro
Copy link
Contributor

traversaro commented Jan 21, 2021

I experienced a failure in a downstream CI (see robotology/robotology-superbuild-dependencies-vcpkg#39) due to the fact that this PR enabled by default the strict resource mode, while before by default the strict mode was set to OFF. While I see the use of the strict option, I think that for the time being it could make sense for it to be opt-in by default for backward compatibility, as any user of vcpkg's ogre port until now could be relying on it, while any user interested in strict mode can enable it if necessary. What do you think @dweckmann @vicroms ?

For more visibility, I opened a PR in #15789 .

seanyen pushed a commit to ms-iot/vcpkg that referenced this pull request Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants