-
Notifications
You must be signed in to change notification settings - Fork 306
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
Miscellaneous fixes #2848
base: main
Are you sure you want to change the base?
Miscellaneous fixes #2848
Conversation
CMakeLists.txt
Outdated
# We need libcurses.so only, so exclude libform.so from the list. | ||
list(FILTER CURSES_LIBRARIES EXCLUDE REGEX ".*libform.*") | ||
list(APPEND LINK_LIBRARIES ${CURSES_LIBRARIES}) | ||
find_library(CURSES_LIB REQUIRED NAMES curses) |
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.
This change is not equivalent to what was before. What about tinfo library in case of dynamic linking?
Even if it works in our environment I recommend double check it. There was an issue related to that.
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.
Right. I have changed this to follow the LLVM logic here
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 the set of link libraries changed for release package or stayed the same as it was?
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.
The release package used and uses the path under ISPC_STATIC_LINK
. So, the find_package(Curses REQUIRED)
and the following logic has never used in the release build. The set of linked libraries hasn't changed.
1d2b88b
to
0d5c824
Compare
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.
What libs do we need to be able to print output in terminal in different colors and detect terminal width (--no-wrap
disables that)?
ISPC uses none libraries to do that. See here Line 68 in 0f2b9fa
Line 87 in 0f2b9fa
Given that it looks like the ncurses dependency is obsolete (#2855). |
Thanks for explaining this. If we can drop this dependency, we should probably do that. But we need to make sure that output works as it used to be. |
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.
Why removing tinfo only for LLVM 18.0+? It makes the logic more complex - i.e it's ISPC version * LLVM version, rather than just ISPC version.
Because I don't want to rebuild the previous versions. |
Up to you, but I vote for removing it once and for all versions - so we get better testing, less problems explaining different requirements when others are building ISPC and simpler code. |
This fixes build-related issue #2847
It fixes
ispc-opt
to check that provided pass name is correct.