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

Add CMake based buildsystem #69

Merged
merged 2 commits into from
May 22, 2020
Merged

Add CMake based buildsystem #69

merged 2 commits into from
May 22, 2020

Conversation

Flamefire
Copy link
Contributor

Based on work by @harryherold

This allows for easier development as e.g. include paths and the like are set up correctly.

While this module could be installed by CMake the preferred approach is still pip, so this is for development only, but IMO very valuable.

@Flamefire
Copy link
Contributor Author

Travis with the ppa seems to be extremly unreliable:

The command "sudo -E apt-add-repository -y "ppa:andreasgocht/scorep"" failed and exited with 1 during .

How about using the same build-and-cache workflow done in my GHA CI (see #68)

@AndreasGocht
Copy link
Collaborator

There are a few questions to this PR:

  • Why is pip install . or pip install -e . not sufficient?
  • What are the benefits of CMake exactly?
  • How does CMake deal with virtual environments?

In general, it's only a hassle to maintain two build systems.

@Flamefire
Copy link
Contributor Author

Flamefire commented May 20, 2020

This is for development. When you just open the repo/folder your include paths will be incomplete and when you do pip install . and get compiler errors they are harder to track down.

The CML solves this: It sets up the correct environment, can generate compile_commands.json usable by IDEs or even full IDE projects. I don't expect virtual environments to be a problem, it works with pyenv at least

As for maintenance: It is not used for regular installation so if it gets broken, the next one using it can fix it, no harm done. And there are 11 lines of code, 8 if you exclude the (non-supported) installation part (which I just included for completeness). Most of them are trivial or boilerplate.

The fact that I'm the second one bothering with adding one seems to indicate demand for this.

@harryherold
Copy link

harryherold commented May 20, 2020

I totally agree with @Flamefire , it is really a nightmare to deal with compiler errors through setup.py and these distutils.core stuff. Especially, when you have to change the default compiler to Intel or something else and you deal with unsupported complier flags and have to change them.

@Flamefire
Copy link
Contributor Author

Ok there is one thing with installation: You'd need to set -DCMAKE_INSTALL_PREFIX=<...> to the prefix of your python installation. This is not done automatically.
However this means it will work with any virtualenv, system, userenv etc. as you just have to set the above config option to what you like. This can be used to create or overwrite and existing installation and then continue testing from within the IDE (I'm using VSCode for example) when you run the installation after every change.
Maybe this could be improved so only a build is required, but IMO just running a pip install ./make install on a console/through IDE before running the tests is fine.

@AndreasGocht
Copy link
Collaborator

This is for development. When you just open the repo/folder your include paths will be incomplete and when you do pip install . and get compiler errors they are harder to track down.

Ok. I understand the Issue. Especially, as you want to address the cProfile interface this might be relevant.

What about creating a separate folder for unsupported build systems? I'd like to avoid people trying the C-Make file, and raising an issue because they did not realise, that CMake is the "not supported" way of installing.
Moreover, it seems reasonable that someone comes along with another build system, which he insists is better than CMake. Having a third or even more build files in the root folder does not seem to be a good idea. And they also could argue, that they just need it for development purposes.

@Flamefire
Copy link
Contributor Author

and raising an issue because they did not realise, that CMake is the "not supported" way of installing.

How about adding a sentence to the Readme instead? Then you can close the issue with RTFM.
A subfolder is not great as it disrupts usual workflow practices which expect a root CML. Again VSCode as an example: Having a root CML sets up everything by just opening the folder.

Moreover, it seems reasonable that someone comes along with another build system, which he insists is better than CMake.

Understand this. However I would just close those and say: We have 1 installation method and 1 build system for development. Those work, period. "Better" is always subjective. So we have those 11 lines of CMake code and if you are not happy with it you can use whatever you like locally.

I don't really care what build system it is, as long as it "just works" for common workflows. So if at all someone can come along and propose replacing CMake. But using CMake has the huge advantage that the existing FindScorep file developed and maintained in other repos can be used.

So I expect your situation to not arise or to be shut down right away. Final "Because": What value does another build system add to make it worth adding especially if you already were contra adding CMake? Here it can be argued that it solves a problem. Another build system solves the same problem differently so no benefit.

@AndreasGocht
Copy link
Collaborator

How about adding a sentence to the Readme instead?

I fear that this might not always help.

Then you can close the issue with RTFM.

Well, I think simply posting RTFM and close is a bit too cruel 😉 . Can you add a warning or something, that can't be overseen, and states that the CMake is for Development only and not supported at all? Best before anything else happens, or with some explicitly accepting or whatever else? (I am open for good Ideas here.)

If someone comes along with a different build system I would add the same requirements together with a deadline for objections, and that only one build system is accepted. It's up to you then, to discuss this with whoever comes along.

@Flamefire
Copy link
Contributor Author

Well, I think simply posting RTFM and close is a bit too cruel

I was (half) joking of course ;)

Done in 3632e04. The message show during the cmake run is:

CMake Warning (dev) at CMakeLists.txt:4 (message):
  CMake is only useful to develop this project, no support is provided.To
  install the module properly use pip, see the Readme for details.
This warning is for project developers.  Use -Wno-dev to suppress it.

Should be enough :)

If someone comes along with a different build system

Sure. I'll ask the question if that provides any benefit over the existing (cmake) solution and if it can't be shown that the benefit is even worth discussing that can be dismissed. So answers like "but I like X better" are invalid.

@AndreasGocht AndreasGocht merged commit 1e6af6e into score-p:master May 22, 2020
@AndreasGocht
Copy link
Collaborator

Merged.

@Flamefire Flamefire deleted the cmake branch May 22, 2020 12:35
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.

3 participants