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

fixed #13083 - correct addons and cfgs install locations #6764

Merged
merged 1 commit into from
Sep 7, 2024

Conversation

AMDmi3
Copy link
Contributor

@AMDmi3 AMDmi3 commented Sep 2, 2024

FILESDIR_DEF is an actual path which should be used for installation, while FILESDIR is an option with default value of OFF which leads to files installed to ${CMAKE_INSTALL_PREFIX}/OFF/{addons,cfgs,platforms}

`FILESDIR_DEF` is an actual path which should be used for installation, while `FILESDIR` is an option with default value of `OFF` which leads to files installed to `${CMAKE_INSTALL_PREFIX}/OFF/{addons,cfgs,platforms}`
@firewave
Copy link
Collaborator

firewave commented Sep 2, 2024

Thanks for your contribution.

This needs tests to make sure it actually behaves as intended.

We also need to get input from the actual packagers how they are building packages (I have been wanted to do that for ages).

See also https://trac.cppcheck.net/ticket/8659.

@firewave
Copy link
Collaborator

firewave commented Sep 2, 2024

CC'ing @c72578 @gdsotirov

@firewave
Copy link
Collaborator

firewave commented Sep 2, 2024

Something also seems wrong when using FILESDIR via make install. A project using Cppcheck is having issues.

@AMDmi3
Copy link
Contributor Author

AMDmi3 commented Sep 2, 2024

This needs tests to make sure it actually behaves as intended.

It does indeed, in fact I'm surprised such regression came unnoticed. IIRC that's not the only installation regression.
I'm not sure how to test the installation process thoughtfully (you don't really want to keep a list of installed files and update it with each release). But it would help if you added cmake --install step in CI, and also a run of installed cppcheck with one of configs on some test code.

We also need to get input from the actual packagers how they are building packages (I have been wanted to do that for ages).

You can have my input as FreeBSD packager, you can also inspect known recipes for packages in many repositories. You would also have more input of GH issues were enabled. For instance, you have test regression in 2.15.0 on FreeBSD, but I cannot afford spending time on reporting to (and then monitoring) third party trackers.

@firewave
Copy link
Collaborator

firewave commented Sep 2, 2024

But it would help if you added cmake --install step in CI, and also a run of installed cppcheck with one of configs on some test code.

I was thinking about that - something I never got around to since I wasn't sure how things are actually to behave. Same for make install. We know which files need to be installed so we can easily generate the file lists for those. This is mainly about checking that the resulting directory structure is correct. Adding a check that it actually loads the installed files would be the icing on the cake (especially with me still cleaning that up).

You can have my input as FreeBSD packager

Nice. I wasn't aware of that. But we also have other packagers which actively give feedback so it would be good if all are on the same page.

For instance, you have test regression in 2.15.0 on FreeBSD, but I cannot afford spending time on reporting to (and then monitoring) third party trackers.

I understand that, but that's kinda what has to be done to keeping things together.

@c72578
Copy link
Contributor

c72578 commented Sep 3, 2024

CC'ing @c72578 @gdsotirov

Concerning Fedora packaging, the current rpm spec file can be found here:
https://src.fedoraproject.org/rpms/cppcheck/blob/rawhide/f/cppcheck.spec

@firewave firewave changed the title Fix addons and cfgs install locations fixed #13083 - correct addons and cfgs install locations Sep 7, 2024
@firewave
Copy link
Collaborator

firewave commented Sep 7, 2024

Until #6776 I did not understand the issue and I was not aware it was just a "typo". I filed https://trac.cppcheck.net/ticket/13083 so it is being tracked.

I will tests in a follow-up PR and also open a backport for 2.15.x.

@firewave firewave merged commit 5ad1d39 into danmar:main Sep 7, 2024
63 checks passed
firewave pushed a commit to firewave/cppcheck that referenced this pull request Sep 7, 2024
`FILESDIR_DEF` is an actual path which should be used for installation,
while `FILESDIR` is an option with default value of `OFF` which leads to
files installed to `${CMAKE_INSTALL_PREFIX}/OFF/{addons,cfgs,platforms}`

(cherry picked from commit 5ad1d39)
@firewave
Copy link
Collaborator

firewave commented Sep 7, 2024

Backport to 2.15.x: #6780.

firewave added a commit that referenced this pull request Sep 7, 2024
`FILESDIR_DEF` is an actual path which should be used for installation,
while `FILESDIR` is an option with default value of `OFF` which leads to
files installed to `${CMAKE_INSTALL_PREFIX}/OFF/{addons,cfgs,platforms}`

(cherry picked from commit 5ad1d39)

Co-authored-by: Dmitry Marakasov <[email protected]>
@c72578
Copy link
Contributor

c72578 commented Sep 8, 2024

CC'ing @c72578 @gdsotirov

Concerning Fedora packaging, the current rpm spec file can be found here: https://src.fedoraproject.org/rpms/cppcheck/blob/rawhide/f/cppcheck.spec

Thanks for the additional information concerning FILESDIR and FILESDIR_DEF in the meantime.
In Fedora we have been using -DFILESDIR=%{_datadir}/Cppcheck in the spec files. That's why the issue did not show up.
It led to [1]:

-- FILESDIR =              /usr/share/Cppcheck
-- FILESDIR_DEF =          /usr/share/Cppcheck

Remark:
FILESDIR is a kind of a mixture between an option and an actual path.
By default it is OFF and if a path is defined using FILESDIR, then this path is set for FILESDIR_DEF:

set(FILESDIR_DEF ${FILESDIR})

[1] See e.g.: https://kojipkgs.fedoraproject.org//packages/cppcheck/2.15.0/1.fc42/data/logs/x86_64/build.log

@firewave
Copy link
Collaborator

firewave commented Sep 8, 2024

Thanks for the additional information concerning FILESDIR and FILESDIR_DEF in the meantime.

We have no documentation of those variables. Since it is not possible to only the custom options() via CMake commands looking at cmake/options.cmake has to double as that - which if people now about it. I think I will add at least a reference to the README.

FILESDIR is a kind of a mixture between an option and an actual path.
By default it is OFF and if a path is defined using FILESDIR, then this path is set for FILESDIR_DEF:

Yeah, I changed that in #6437 so I can actually disable it. But even though I authored it I am a bit confused by it. Somehow I totally blanked on that you can set CMake variables without having an option(). And the logic also seems a bit off. It definitely needs the path validation TODO fixed.

Seems my long-standing health issue affect my mind even more than I even realize in some cases...having that many balls in the air definitively doesn't help. I should have added tests in that PR.

@gdsotirov
Copy link
Contributor

I'm already using FILESDIR and it is fine for me.

@AMDmi3 AMDmi3 deleted the patch-1 branch September 18, 2024 13:12
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.

4 participants