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

Miscellaneous fixes #2848

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

nurmukhametov
Copy link
Collaborator

This fixes build-related issue #2847

It fixes ispc-opt to check that provided pass name is correct.

@nurmukhametov nurmukhametov linked an issue May 10, 2024 that may be closed by this pull request
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)
Copy link
Collaborator

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.

Copy link
Collaborator Author

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

Copy link
Collaborator

@aneshlya aneshlya May 13, 2024

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

@dbabokin dbabokin left a 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)?

@nurmukhametov
Copy link
Collaborator Author

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

int ispc::TerminalWidth() {
and here
static bool lHaveANSIColors() {

Given that it looks like the ncurses dependency is obsolete (#2855).

@dbabokin
Copy link
Collaborator

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.

Copy link
Collaborator

@dbabokin dbabokin left a 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.

@nurmukhametov
Copy link
Collaborator Author

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.

@dbabokin
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Report the absence of ncurses library on CMake stage
3 participants