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: don't hard-code absolute path name /etc #1558

Merged
merged 1 commit into from
Apr 9, 2024

Conversation

jktjkt
Copy link
Contributor

@jktjkt jktjkt commented Apr 8, 2024

cmake: don't hard-code absolute path name /etc

Also, don't make this installation conditional on the existence of /etc. Whether or not that directory exists on a build system has no meaning for installation to a target system. This had a potential to break cross-compiling.

- this breaks packaging and installation into a prefix in general
- there's a GNUInstallDirs variable for /etc, too

Also, don't make this installation conditional on the existence of /etc.
Whether or not that directory exists on a build system has no meaning
for installation to a target system. This had a potential to break
cross-compiling.
@jktjkt jktjkt force-pushed the fix-pam-install branch from c555ed5 to a8658ad Compare April 8, 2024 10:31
@michalvasko
Copy link
Member

Okay, but like always, please target the devel branch. Also, in case PAM is not installed on the system, should the config file be copied anyway?

@jktjkt jktjkt changed the base branch from master to devel April 9, 2024 08:26
@jktjkt
Copy link
Contributor Author

jktjkt commented Apr 9, 2024

Okay, but like always, please target the devel branch.

oops, sorry, fixed

Also, in case PAM is not installed on the system, should the config file be copied anyway?

There's no harm in doing that; it's a single text file and it does not bring any extra dependencies. If there's a downstream package which does not like that file, they are free to not include it in a package.

@michalvasko
Copy link
Member

Okay but why remove the directory existence check? Because of cross-compilation?

@jktjkt
Copy link
Contributor Author

jktjkt commented Apr 9, 2024

Okay but why remove the directory existence check? Because of cross-compilation?

Yes, this is explained in the PR description and in the commit message. In fact, this isn't just about cross-compiling. If you have a build-time check like this, you're introducing a non-explicit dependency into your build process. This might be fine when building on a developer's laptop, but in any real, automated packaging solution, this implies a need for a "useless" build-time dependency on libpam, otherwise you have a risk of producing a non-complete package.

@michalvasko
Copy link
Member

Fine, I suppose.

@michalvasko michalvasko merged commit a349281 into CESNET:devel Apr 9, 2024
6 checks passed
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.

2 participants