-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
[fftw3] Fix the build error and usage of feature openmp #35483
base: master
Are you sure you want to change the base?
Conversation
|
||
if (OPENMP_FOUND) | ||
- add_library (${fftw3_lib}_omp ${fftw_omp_SOURCE}) | ||
+ add_library (${fftw3_lib}_omp ${SOURCEFILES} ${fftw_omp_SOURCE}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, but this seems incorrect to me because it's going to duplicate builds of all those other files, and:
add_library (${fftw3_lib} ${SOURCEFILES})
# ...
target_link_libraries (${fftw3_lib}_omp ${fftw3_lib})
means symbols declared by those files should already be available to ${fftw3_lib}_omp
.
I observe that fftw3 spams these functions out with 'X macros' ; is it possible the actual problem is that something else disagrees between the build of fftw3_lib
and fftw3_lib_omp
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait I think I see the problem now. ${fftw3_lib}
is being built as a DLL that doesn't correctly export these. I'm still not sure repeating their definitions in the DLL is the correct outcome...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has this been reported upstream? Worst case the feature could be marked with "supports": "!windows | static"
.
Block Windows and dynamic is: !(windows & !static)
.
Demorgan to get rid of parens: !windows | static
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I submitted an upstream issue: FFTW/fftw3#342, waiting for the upstream's response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is always difficult to report vcpkg issues to upstream unless fully understanding the modifcations which are already in vcpkg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And upstream has very little activity. Still waiting for FFTW/fftw3#331.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is true that the upstream response is slow. If there is no response, I will add supports
for this feature next week.
... from 2020
... from 2017. Is this still an active issue? |
Yes, this issue still exists, |
Fixes #10930, fix the build error and usage of feature
fftw3[openmp]
.About the build error, the related upstream issue: FFTW/fftw3#120.
Feature
fftw3[openmp]
passed with following triplets:Usage test passed on
x64-windows
.SHA512s are updated for each updated downloadThe "supports" clause reflects platforms that may be fixed by this new versionAny fixed CI baseline entries are removed from that file.Any patches that are no longer applied are deleted from the port's directory../vcpkg x-add-version --all
and committing the result.