[ffmpeg] Correctly set environment variables for gcc/clang/icc#6694
[ffmpeg] Correctly set environment variables for gcc/clang/icc#6694cbezault merged 3 commits intomicrosoft:masterfrom
Conversation
ports/ffmpeg/portfile.cmake
Outdated
| endif() | ||
|
|
||
| set(ENV{INCLUDE} "${CURRENT_INSTALLED_DIR}/include;$ENV{INCLUDE}") | ||
| if($CMAKE_CXX_COMPILER_ID STREQUAL "MSVC") |
There was a problem hiding this comment.
I do not understand the requirement for the if/else case here. I thought you defined SEP to get rid of those. You probably need to define two other for ENV{INCLUDE} vs ENV{CPATH} and ENV{LIB} vs ENV{LIBRARY_PATH}. But from my viewpoint this should all be doable in the first if where SEP is defined to get rid of the other 3 ifs below it. The trick will be ENV{${ENV_INCLUDE_VAR}} and setting ENV_INCLUDE_VAR either to INCLUDE or CPATH
There was a problem hiding this comment.
Yeah that's a reasonable refactor. The SEP already got rid of some of those ifs, GNU expects the LIBRARY_PATH var to have semicolons when targeting Windows.
|
yeah the d suffixes.... sometimes they break stuff if they are there, sometimes they are necessary for find_package to work correctly. Really annoying. PR 5543 will only ever have zero regression from a CMake perspective if all find_package calls which define a _DEBUG variable are correctly found with the correct debug suffix. The reason I made that a FATAL_ERROR and submited PRs to correct the suffixes is because it is the only clean way for external projects to have correct library linkage in a multi config generator like VS without adding unnecessary extra code to VCPKG. |
|
and bzip requires the extra d: From CMakes FindBZip2 module |
|
We're going to use bzip as a test case to see if we can still support multi-config generators while also not having the d suffix and also not braking the built-in CMake modules using wrappers. |
|
That is possible if you override |
|
Also there was a recent issue to add a d to a library/dll name because the user had both debug and release dlls in the same folder. Although it is probably a really bad idea to install both into the same folder, users seem to have these setups. (See issue #4878) |
|
/azp run |
|
Pull request contains merge conflicts. |
|
/azp run |
1 similar comment
|
/azp run |
|
@cbezault, this breaks build for I'm getting an error while trying to build with Contents of Here's what is at the end of PS: the issue resolves after reverting to the parent commit (f2e1c52). |
|
Hmmmmm, it built fine on the CI, what version of VS are you running? |
|
VS 2019. |
|
An excerpt from the CI build log you are referring to: I guess it builds default features only, since |
|
Yup, I was about to say. We really need some way to test features. |
|
I'm away from a computer right now but I'd like to try changing line 34 of the portfile.cmake to just "if(MSVC)" and try again when I'm back at a computer. If you'd like to do it now I'd appreciate it. |
|
Nope, doesn't help. |
|
Sigh, okay I'll add this to the list of things to investigate. |
Should enable features to actually work on non-msvc compilers for non-windows targets.
Also remove bzip2 patch from #6570 , the bzip2 feature will be fixed by a PR for the bzip port.
Seems like a lot of the features are broken because of the d suffixes on other ports, will be working on getting them all up and running.