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

[cmake] Correctly install the library into the system #25

Closed
wants to merge 11 commits into from

Conversation

@cryptomilk
Copy link
Contributor

commented Sep 7, 2018

This patchset cleans up cmake so that the library can be correctly packaged for distributions. It also cleans up cmake as there are several things which should not be done in cmake.

See also:
https://pabloariasal.github.io/2018/02/19/its-time-to-do-cmake-right/

The package is built here:
https://build.opensuse.org/package/show/home:gladiac:rocm/hsakmt-roct

@fxkamd

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2018

I think part of this (or something equivalent) will be in the upcoming ROCm 1.9 release: using GNUInstallDirs, installing a pkgconfig file, and some reorganization of the dev package and how the headers are installed. I think we're still putting the pkgconfig in the wrong place, I'll look info fixing that.

Can you explain the reasoning behind the changes not to mess with CMAKE_SHARED_LINKER_FLAGS and CMAKE_C_FLAGS? Excuse my ignorance, I'm not a cmake expert.

Thanks,
Felix

@cryptomilk

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2018

Those variables are there to be modified from the cmake command line, they should not be touched. Also they are used by everything and could influence configure checks in a negative way!

If you want to set compiler flags, use:

target_compile_options()

if you want to change linker flags, use:

set_property(TARGET PROPERTY LINKER_FLAGS ...)

The same for include_directories(), you should use target_include_directories() and only specify the once needed for that target. Less include directories means faster compilation :-)

You find more details if you read:
https://pabloariasal.github.io/2018/02/19/its-time-to-do-cmake-right/

Which gives you a nice overview and summery.

I would give you more advice if you would develop your projects in the open to deduplicate work. This way people start to fix bugs and you tell them, sorry you wasted your time we fixed that already. :-(

@fxkamd

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2018

Thank you, that blog post was an interesting read. If done right (exported targets), CMake users wouldn't even need our pkg-config file any more. I think I also see some hints for how to better reconcile the differences between our internal build systems and how end users build our code.

I'll propose publishing our internal development to the master branch outside of the ROCm release cycle. I think a daily automatic push from our internal branch to GitHub should be possible. I'll have to think more about how to integrate GitHub pull request with our internal code review and test workflow. Or get our DevOps team to think about it for me. ;)

@cryptomilk

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2018

Also for finding libraries and header files, like for libnuma, you should create a find module.

Here is a simple script which creates one for you:
https://xor.cryptomilk.org/scripts/cmake_create_findpackage

I've just converted one of my projects to modern cmake. You might want to take a look how to check for compiler flags support :-)
https://git.cryptomilk.org/users/asn/cmocka.git/tree/

@cryptomilk

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2018

## If the library is a release, strip the target library
if ( "${CMAKE_BUILD_TYPE}" STREQUAL Release )
    add_custom_command ( TARGET ${HSAKMT_TARGET} POST_BUILD COMMAND ${CMAKE_STRIP} ${HSAKMT_COMPONENT}.so )
endif ()

This is also very bad, as distributions want to build Release, but create separate debuginfo packages. But you can't do that if they are already stripped.

Please remove this code and use 'make install/strip instead!

@cryptomilk

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2018

Should I update the patch for 1.9? It is still valid.

@cryptomilk cryptomilk force-pushed the cryptomilk:asn/cmake branch from 93dd073 to 66e43ba Sep 17, 2018
@kentrussell

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2018

I'd appreciate you rebasing it onto 1.9. That way it will be easier to get it into the internal branch and included either in 1.9.2 or 2.0. Thank you!

@cryptomilk

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

It already has been rebased.

kentrussell added 4 commits Nov 26, 2018
The feature hasn't been ported to the release branch, so remove it from
execution

Change-Id: I4baf0b1ebf610a0ed45304a773d88b4b57a3e951
This won't run on any ASIC, not just Vega10, so it should be removed
from execution on all GPUs

Change-Id: Iaad383d383d970e88b19a505050c6ba052ed683a
This is intermittently causing VM faults and excessive evictions, which
causes the rest of the tests to fail. Take it out for now until someone
can investigate

Change-Id: I9c43890bc9f03a4a31efbc18df0df5e40a232c58
 1) Temporarily remove SDMA tests from VG20
 2) Add ZeroInitializationVram test to SDMA blacklist
 3) Add more SDMA-related tests to SDMA_BLACKLIST

Change-Id: I1abab1e7aff910fc2e21c3ce91c61684534fc708
@candrews

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2018

What's the latest news on this PR?

I'm packaging ROCT-Thunk-Interface for Gentoo, and having this fix to the build system is critical :)

Thanks!

@cryptomilk

This comment has been minimized.

Copy link
Contributor Author

commented Dec 14, 2018

Craig, I think they wont change it before the 2.0 release. However AMD and an internal development an an external development which makes it hard to contribute.

As there is no real answer what happens with those fixes I didn't do any further packaging. The basic libs are easy but I need OpenCL and that is a big monster. So until AMD starts to adopt Modern CMake and works with us to get this correctly implemented we should just wait.

@kentrussell kentrussell force-pushed the RadeonOpenCompute:master branch from 238782c to 3ba20ce Dec 14, 2018
@jlgreathouse

This comment has been minimized.

Copy link

commented Dec 21, 2018

Hi @cryptomilk @candrews

Somewhat related to the issues being raised here, if you're doing building/packaging of ROCm for various Linux distributions, you may be interested in the Experimental ROC project we just released.

One of the projects in that repo is scripts to rebuild and package ROCm from our GitHub source for various Linux distros. Right now they're focused on Ubuntu, CentOS, and Fedora, but I would definitely be interested in any modifications that would help bring up ROCm for Gentoo, OpenSUSE, etc. If you're working getting ROCm up on one of these other distros, these scripts may be a good starting point for some of the more harsh steps (unfortunately) needed to build/package some of our projects.

cryptomilk added 7 commits Sep 7, 2018
This will allow distributions to install it correctly.
Distributions want to generate debuginfo packages, do not strip them! If
you want to do it during installation use 'make install/strip'!
Another cmake project like hsa-runtime could just use:

find_package(hsakmt REQUIRED 1.9.0)
@cryptomilk cryptomilk force-pushed the cryptomilk:asn/cmake branch from 86b29b2 to 87a0901 Dec 21, 2018
@cryptomilk

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

HI @jlgreathouse thank you very much for the pointer.

However those scripts do not follow distribution packaging guidelines. For packaging ROCm for distributions we need clean cmake files to compile and install the software correctly on a Linux system. See my modifications.

As you don't really do development in the open all I can do is to rebase these fixes with each release until someone from AMD looks into how to do cmake correctly.

Here is my start with packaging, currently openSUSE only but enabling Fedora is just a few clicks away ...

https://build.opensuse.org/project/show/home:gladiac:rocm

I didn't more packaging as someone said developers will look into cmake but obviously nobody cared ...

So what are AMDs plans?

@jlgreathouse

This comment has been minimized.

Copy link

commented Dec 21, 2018

Hi @cryptomilk

I'm aware that the scripts in the Experimental ROC repo do not follow any particular distro's packaging guidelines. My goal for them focused more on generality across distributions so that the community could have a readable and understandable starting point for any of their own work. I do not expect these scripts to be used in any distro's build system, and I also don't really have the bandwidth to come up to speed on the packaging rules for dozens of different distros. :)

That said, I'm still interested in any help with porting these scripts to new distros. In the medium term, that repoistory will also contain patches to enable experimental features within the ROCm software stack. Being able to download, patch, and build each of the modified ROCm software layers to enable these features is another I'd like to have the build tools working across various distros.

Setting aside that repo for a second:
The various projects in the ROCm project develop in different ways. Some of this is historical (e.g. the amdgpu graphics driver and the OpenCL runtime have been in development longer than ROCm has existed, so their development practices were already in place). Some is due to institutional inertia (e.g. a new project is started, and all of the engineers working on it are used to a particular development style).

At the moment, you're focusing on the parts of the ROCm stack that are more heavily developed internally. I believe many of the other projects (e.g. HCC, HIP, our LLVM compiler, our ROCm libraries) are developed primarily on GitHub and more rapidly look at PRs.

@cryptomilk

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

Joseph,

you don't have to as normally packagers from distros are taking care of maintaining packages. However the build system should work and currently it does a lot of strange things. I've tried to fix them for this project and stopped doing it for others. If they don't get applied for this project it doesn't really make sense to start it for others :-)

The current patchset of the PR just fixes the most ugly things that the lib can be build and installed correctly on any Linux system. The question is what are your plans, this PR is open for a few month now. It is still unclear if these patches will get accepted or if someone will fix the issues differently.

@jlgreathouse

This comment has been minimized.

Copy link

commented Dec 21, 2018

Towards your question about AMD's plans and whether or not folks are looking at fixing their cmake files:

I believe that teams are interested in this (after putting together all of the required scripts to actually do these builds, I know I certainly am). That said, just as a practical matter, I'd wager any PRs for this are going to have to happen in a piecemeal manner. I'll admit that most of the ROCm developers are by no means cmake experts. PRs that demonstrate the suggested changes, with some reasoning behind those changes will definitely be helpful.

The main hump to get over for each of the projects is that, if the "internal" cmake system works for building ROCm, it can difficult to get time on a developer's plate to fix it for external release. That said, I've not yet run into a developer that will purposefully decline a reasonable changes to simplify their build system.

Perhaps splitting the PRs into smaller chunks would help, so that it's not an "all or nothing" request.

Part of the difficulty with the more "internally developed" projects is that the resulting cmake system needs to still work for our internal builds. So splitting things up might make it easier to slowly pull things in, test on the internal build servers, etc.

@cryptomilk

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

There is always 'git cherry-pick' and I'm happy to split this up into several PRs if there is need to. Till now nobody seemed to really look into them. The changes should not break any of your internal scripts to create packages.

If someone speaks up to work on fixing this stuff, I will create corresponding PRs and close that one. So who will do the work on AMDs side?

@kentrussell kentrussell force-pushed the RadeonOpenCompute:master branch from 3ba20ce to c65f2de Feb 5, 2019
@tstellar

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

I'm packaging the thunk for Fedora and have some similar patches that I need to apply. What's the best way forward here? Should we try to split the patches up into separate PRs?

@fxkamd

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

Some of the weird things we do is to split the headers and binaries into separate packages. The patches look like they remove that ability. But there maybe something I'm missing. Does the COMPONENT keyword allow building packages that only include some components? That would be much cleaner than what we currently do.

@tstellar

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

@fxkamd Which part of the change do you think makes separating the headers and binaries not work?

Would it be better if we split the CMAKE_C_FLAGS and CMAKE_SHARED_LINKER_FLAG changes out into a separate pull request. Those changes should not change the default build at all, but are important for distro packaging.

@cryptomilk

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

I could create a PR for each patch if that is preferred ...

@kentrussell

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

Thanks @cryptomilk , that would be great. I'll get on integrating it internally next week, to ensure that our test infrastructure doesn't break due to the changes.

@cryptomilk

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

I've created separate pull requests. Closing this one.

@cryptomilk cryptomilk closed this Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.