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

[mathgl] missing internal dep #31013

Merged
merged 2 commits into from
Apr 26, 2023

Conversation

autoantwort
Copy link
Contributor

Fixes

CMake Error at CMakeLists.txt:605 (message):
  You have to enable ZLib if you plan to use PNG export.

@jimwang118 jimwang118 added the category:port-bug The issue is with a library, which is something the port should already support label Apr 23, 2023
@jimwang118
Copy link
Contributor

There is a problem with the usage of this port. Follow the usage prompt to test the compiled library and the following problems occur:

Severity	Code	Description	Project	File	Line	Suppression State
Error (active)	E1696	cannot open source file "mgl2/dllexport.h"	testmathgl.exe (testmathgl\Debug\testmathgl.exe) - x64-
Error	C1083	Cannot open include file: 'mgl2/dllexport.h': No such file or directory 	

@autoantwort
Copy link
Contributor Author

Follow the usage prompt to test the compiled library and the following problems occur:

Was this caused by this PR or is this an existing problem?

@jimwang118
Copy link
Contributor

Follow the usage prompt to test the compiled library and the following problems occur:

Was this caused by this PR or is this an existing problem?

This should be a problem that has always existed. I tested the version before the upgrade and this problem also exists.

@BillyONeal
Copy link
Member

BillyONeal commented Apr 25, 2023

This should be a problem that has always existed. I tested the version before the upgrade and this problem also exists.

Then I don't think we should be requiring that @autoantwort fix it in order to fix this unrelated bug. Did you see other problems?

BillyONeal
BillyONeal previously approved these changes Apr 25, 2023
@jimwang118
Copy link
Contributor

You can make a patch that modifies the src/CMakeLists.txt file. Add the following two lines to use it normally.

target_include_directories(mgl PUBLIC $<INSTALL_INTERFACE:include>)
target_include_directories(mgl-static PUBLIC $<INSTALL_INTERFACE:include>)

@BillyONeal
Copy link
Member

You can make a patch that modifies the src/CMakeLists.txt file. Add the following two lines to use it normally.

target_include_directories(mgl PUBLIC $<INSTALL_INTERFACE:include>)
target_include_directories(mgl-static PUBLIC $<INSTALL_INTERFACE:include>)

I'm not sure I understand. Are you saying this fixes the preexisting usage problem? Do you already have that change in a branch somewhere?

I'm happy to smash a spot fix like that into the same PR if @autoantwort is willing but I don't want to block them over it.

@jimwang118
Copy link
Contributor

You can make a patch that modifies the src/CMakeLists.txt file. Add the following two lines to use it normally.

target_include_directories(mgl PUBLIC $<INSTALL_INTERFACE:include>)
target_include_directories(mgl-static PUBLIC $<INSTALL_INTERFACE:include>)

I'm not sure I understand. Are you saying this fixes the preexisting usage problem? Do you already have that change in a branch somewhere?

I'm happy to smash a spot fix like that into the same PR if @autoantwort is willing but I don't want to block them over it.

I cloned the branch of this PR locally, and then used the above method to fix the usage issue and the local test can be used normally. If @autoantwort does not want to incorporate this usage issue in this PR, then I will submit a separate PR to fix the usage issue.

@dg0yt
Copy link
Contributor

dg0yt commented Apr 26, 2023

@jimwang118 I can tell you that this is broken for many of the unofficial configs added in vcpkg. You can find suspects with git grep.

@autoantwort
Copy link
Contributor Author

@jimwang118 I have now included your patch. Thank you!

@jimwang118 jimwang118 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Apr 26, 2023
@BillyONeal BillyONeal merged commit 1fcceb3 into microsoft:master Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants