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

parallelstl: install CMake config files #53751

Closed
1 of 4 tasks
omor1 opened this issue Apr 26, 2020 · 15 comments
Closed
1 of 4 tasks

parallelstl: install CMake config files #53751

omor1 opened this issue Apr 26, 2020 · 15 comments
Labels
outdated PR was locked due to age upstream issue An upstream issue report is needed

Comments

@omor1
Copy link

omor1 commented Apr 26, 2020

Checklist

  • ran brew update and can still reproduce the problem?
  • ran brew doctor, fixed all issues and can still reproduce the problem?
  • ran brew gist-logs <formula> (where <formula> is the name of the formula that failed) and included the output link?
  • if brew gist-logs didn't work: ran brew config and brew doctor and included their output with your issue?

What you were trying to do (and why)

The parallelstl formula doesn't use CMake to generate and install the ParallelSTLConfig.cmake file. This makes using the Parallel STL library more difficult than it ought to be.

What happened (include command output)

Parallel STL is missing the CMake config file after installation via brew.

What you expected to happen

Installing Parallel STL would fully install Parallel STL.

@omor1
Copy link
Author

omor1 commented Apr 26, 2020

brew doctor tells me that the command line tools are supposedly out-of-date; System Preferences does not show me an update. This prevents me from running brew gist-logs. In any case, this is very obviously a problem with the formula, given that it doesn't generate a bottle.

Bo98 added a commit to Bo98/homebrew-core that referenced this issue Apr 26, 2020
@Bo98
Copy link
Member

Bo98 commented Apr 26, 2020

Should be fixed now.

@omor1
Copy link
Author

omor1 commented Apr 27, 2020

You forgot to bump the revision!

@Bo98
Copy link
Member

Bo98 commented Apr 27, 2020

Indeed - I'll go bump it. You can do brew reinstall parallelstl in the meantime.

@omor1
Copy link
Author

omor1 commented Apr 27, 2020

I've already done that :). Thanks!

@omor1
Copy link
Author

omor1 commented Apr 27, 2020

Using pstl::ParallelSTL from CMake seems to be problematic in the current setup. The replacement STL headers are installed in the non-standard stdlib directory inside the parallelstl Cellar directory, but this isn't linked into /usr/local/stdlib. It seems that ParallelSTLTargets.cmake finds the install directory based on ${CMAKE_CURRENT_LIST_FILE}, which I guess doesn't resolve the symbolic link to the Cellar directory, so it tries to find stdlib in /usr/local rather than in /usr/local/Cellar/parallelstl/20200330.

I'm not sure what the solution here is. Installing the stdlib directory is inelegant, but should work.

@Bo98
Copy link
Member

Bo98 commented Apr 27, 2020

Upstream's CMake files don't even install stdlib by default. I had to do that bit manually. And since it isn't in a standard Unix location - it doesn't get symlinked to /usr/local. (You can find it in /usr/local/opt/parallelstl/stdlib, but that probably doesn't help you.) There is strict enforcements on what gets symlinked to /usr/local.

I think the solution would be for it to be in a standard location, such as include/parallelstl/stdlib or maybe even something in share. I suspect upstream don't give that flexibility though.

@omor1
Copy link
Author

omor1 commented Apr 27, 2020

I'm as flummoxed as you are. The separation of the replacement STL files into stdlib only happened with the last two releases. Maybe installing it into /usr/local/Cellar/parallelstl/<version>/include/pstl/stdlib? That would likely require a patch to CMakeLists to change the include directory INSTALL_INTERFACE. Mostly it seems that upstream is problematic for this.

@Bo98
Copy link
Member

Bo98 commented Apr 27, 2020

I could mess around with patching the CMakeLists but I'd like it to not be something that sticks around forever. It would be worth making a bug report upstream first and see if there's any initial thoughts there.

@omor1
Copy link
Author

omor1 commented Apr 27, 2020

I'm working on that right now.

@omor1
Copy link
Author

omor1 commented Apr 27, 2020

For now I'm working around this by using -DParallelSTL_ROOT=/usr/local/opt/parallelstl when configuring CMake.

@omor1
Copy link
Author

omor1 commented Apr 27, 2020

In the meantime, should we open this issue back up?

@Bo98
Copy link
Member

Bo98 commented Apr 27, 2020

We generally don't for upstream issues but I'll still be receiving notifications for this issue and may re-open when there's action to be done on our side.

If you need any further evidence for upstream, vcpkg were also forced to have to patch upstream's scripts to actually install stdlib and to do so in a standard location include/pstl: https://github.com/microsoft/vcpkg/blob/020923a98dac40b55098170ae3dcb65a4eab58b5/ports/parallelstl/fix-cmakelist.patch

@Bo98 Bo98 added the upstream issue An upstream issue report is needed label Apr 27, 2020
@omor1
Copy link
Author

omor1 commented Apr 27, 2020

That's a good catch, I'll make note of it on oneapi-src/oneDPL#33.

@Bo98
Copy link
Member

Bo98 commented Apr 27, 2020

Yeah, it's good to have other package managers to compare to - I didn't want them to get all caught up in the Homebrew symlink stuff.

I'm pretty sure Debian (if they were to have parallelstl) would also reject anything that expected things in /usr/local outside of standard locations, and would also likely raise a bug report if the install script didn't actually install everything.

@lock lock bot added the outdated PR was locked due to age label Jun 24, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age upstream issue An upstream issue report is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants