Skip to content

handle GNUInstallDirs /opt special case in CMakeMake easyblock#2105

Merged
lexming merged 1 commit intoeasybuilders:developfrom
migueldiascosta:cmake_gnuinstalldirs_opt
Aug 5, 2020
Merged

handle GNUInstallDirs /opt special case in CMakeMake easyblock#2105
lexming merged 1 commit intoeasybuilders:developfrom
migueldiascosta:cmake_gnuinstalldirs_opt

Conversation

@migueldiascosta
Copy link
Copy Markdown
Member

@migueldiascosta migueldiascosta commented Jul 21, 2020

@migueldiascosta
Copy link
Copy Markdown
Member Author

migueldiascosta commented Jul 24, 2020

see also https://github.com/easybuilders/easybuild-easyconfigs/pull/9137/files and easybuilders/easybuild-easyconfigs#9137 (comment)

@boegel boegel changed the title handle GNUInstallDirs /opt special case in cmake easyblock handle GNUInstallDirs /opt special case in CMakeMake easyblock Aug 5, 2020
@lexming
Copy link
Copy Markdown
Contributor

lexming commented Aug 5, 2020 •
edited
Loading

I can reproduce the issue: https://gist.github.com/lexming/cef69fe274088f60395306835cd65e49

The installation eb --installpath-software=/opt/ebtest k/kim-api/kim-api-2.1.3-foss-2019b.eb failed during the install step with

CMake Error at completions/cmake_install.cmake:49 (file):
  file cannot create directory:
  /etc/opt/ebtest/kim-api/2.1.3-foss-2019b/bash_completion.d.  Maybe need
  administrative privileges.

@lexming
Copy link
Copy Markdown
Contributor

lexming commented Aug 5, 2020

Positive test with this PR: https://gist.github.com/lexming/6abbd018382fe46f10fec33b758ea352

Installing kim-api with eb --installpath-software=/opt/ebtest k/kim-api/kim-api-2.1.3-foss-2019b.eb, puts the dir bash_completion.d in /opt/ebtest/kim-api/2.1.3-foss-2019b/etc/bash_completion.d/.

Installations outside /opt work as usual.

@lexming lexming added this to the next release (4.2.3?) milestone Aug 5, 2020
lexming
lexming approved these changes Aug 5, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@lexming lexming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lexming
Copy link
Copy Markdown
Contributor

lexming commented Aug 5, 2020

Going in, thanks @migueldiascosta !

@lexming lexming merged commit e971fc8 into easybuilders:develop Aug 5, 2020
@lexming lexming mentioned this pull request Aug 5, 2020
Closed
boegel
boegel reviewed Aug 14, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(looks like I never submitted this review, ugh... doing so now, may need to be followed up)

Comment thread easybuild/easyblocks/generic/cmakemake.py

if self.installdir.startswith('/opt'):
# https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html
options.append("-DCMAKE_INSTALL_SYSCONFDIR=%s/etc" % self.installdir)
Copy link
Copy Markdown
Member

@boegel boegel Aug 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@migueldiascosta Don't we need to set CMAKE_INSTALL_FULL_SYSCONFDIR instead?

Likewise for LOCALSTATEDIR and RUNSTATEDIR?

Should we also do something special in case the installation prefix starts with /usr?

Copy link
Copy Markdown
Member Author

@migueldiascosta migueldiascosta Aug 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

w.r.t. to CMAKE_INSTALL_FULL_<dir>, it's generated from CMAKE_INSTALL_<dir> and it will only be different from it if this value is not an absolute path, and we're setting it to an absolute path, so it's correct, no?

Copy link
Copy Markdown
Member

@boegel boegel Aug 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I got a bit confused by the wording, should be OK.

Copy link
Copy Markdown
Member Author

@migueldiascosta migueldiascosta Aug 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

w.r.t. the rest, see #2124

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@boegel boegel boegel left review comments

@lexming lexming lexming approved these changes

Assignees

No one assigned

Labels

bug fix

Projects

None yet

Milestone

4.3.0

Development

Successfully merging this pull request may close these issues.

3 participants

@migueldiascosta @lexming @boegel
⚡