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

settings.conf installed in strange place #2075

Closed
jriddell opened this issue Nov 17, 2022 · 7 comments
Closed

settings.conf installed in strange place #2075

jriddell opened this issue Nov 17, 2022 · 7 comments
Labels

Comments

@jriddell
Copy link
Member

There is a file settings.conf which is installed into the build directory

8f72d25

https://github.com/calamares/calamares/blob/calamares/CMakeLists.txt#L589

e.g. when I build locally it gets installed into /home/jr/src/extras/calamares/github/calamares/build/settings.conf instead of /usr/share/calamares or similar

What is this madness?

@kkofler
Copy link
Contributor

kkofler commented Nov 17, 2022

+1, this breaks distribution workflows, see also:
https://src.fedoraproject.org/rpms/calamares/blob/rawhide/f/calamares-3.2.61-default-settings.patch

I do not see why it is not a reasonable thing to do to apply a patch to the upstream default settings adapting them to the distribution and then install them as part of the Calamares package. IMHO, that is a perfectly valid way to handle configuration files, and surely easier to maintain than having to copy the whole bunch of config files into a custom noarch package.

Please revert commit 8f72d25.

@dalto8
Copy link
Contributor

dalto8 commented Nov 17, 2022

Personally, I think this change fixes more than it breaks. Too many people were doing the wrong thing here.

While I think the approach of patching all the config files on the distro side is sub-optimal, if that is the approach you prefer you can also patch the CMakelists to install them again or simply install them as part of your package.

@jriddell
Copy link
Member Author

From the deploy guide
https://github.com/calamares/calamares/wiki/Deploy-Guide

Distributions are strongly urged to package configuration files separately from Calamares itself, and to install them in /etc/calamares. It is not recommended to edit or patch the configuration files included with Calamares for production use -- they are examples.

That is fair enough. I just don't understand why it's installing a file into the build directory, that's broken.

@demmm demmm added the bug label Nov 17, 2022
@killajoe
Copy link
Contributor

killajoe commented Dec 5, 2022

this is installed by the package and something is creating the file so it will get into the package build..
Screenshot_2022-12-05_17-08-00
looks like the path where I was building calamares into..

@kkofler
Copy link
Contributor

kkofler commented Dec 18, 2022

How does that commit address this issue? settings.conf is still installed to the same weird location, just using a different CMake command at a different spot in CMakeLists.txt.

@kkofler kkofler reopened this Dec 18, 2022
pvshvp-oss pushed a commit to RebornOS-Team/calamares-core that referenced this issue Dec 28, 2022
Having an up-to-date settings.conf in the build directory
makes `calamares -d` in that directory much more predicatable.

This should not have used CMake command `install()`.

FIXES calamares#2075
CLOSEs calamares#2079
@adriaandegroot
Copy link
Contributor

/close

The problem was, as Jon reported, that a file was being install()ed into the build directory. That breaks packaging whenever the packaging depends on CMake's installation logic -- in the face of make install DESTDIR=/tmp/package for instance, where you would get /tmp/package/home/....src/calamares/build/settings.conf . This has been resolved.

Copying settings.conf into the build directory is intentional: it's a developer convenience that calamares -d when run in the build directory picks up the settings (all of them, and updated when edited) from the source repo.

@kkofler
Copy link
Contributor

kkofler commented Feb 20, 2023

I still think commit 8f72d25 should be reverted though. Why does it hurt to have an INSTALL_CONFIG option that is off by default, so that distributions that want it (and that already patch the config files to do the right thing on their distribution) have to only set one CMake variable (through the CLI or a one-line patch) rather than to patch a whole bunch of files to readd code that was once there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants