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

Correct CMake install locations on *nix #62234

Merged
merged 1 commit into from
Jan 9, 2023

Conversation

perryprog
Copy link
Contributor

Summary

None

Purpose of change

PR #61225 changed the location some stuff got installed in, which while correct for the Windows style install location, is incorrect for the default *nix style install locations. (CMAKE_INSTALL_DIR is by default /usr/local instead of c:/Program Files/${PROJECT_NAME}/ according to here.)

Describe the solution

Adjust these install locations to be correct for both OS's, hopefully. I'm not sure what the install locations for gfx/ and doc/ are intended to be for Windows, so I've set it to what it was previously, which is under DATA_PREFIX. I'd like it if this can be confirmed before the PR is merged so I can make the default install locations for both of those directories also depend on OS.

Describe alternatives you've considered

Testing

Builds and runs on macOS. I'd appreciate it if someone on Windows can test.

Additional context

CC @alef: I'd like to know what would be the correct directories according to what you were trying to do.

@github-actions github-actions bot added Code: Build Issues regarding different builds and build environments json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Nov 15, 2022
@alef
Copy link
Contributor

alef commented Nov 15, 2022

The CMake builds do not install correctly, it should be fixed in #61880.
I suggest you rebase this PR after mine gets merged (@akrieger is back)

@akrieger
Copy link
Member

I mean I'm still on vacation but I found time to hop on discord today :p

@perryprog
Copy link
Contributor Author

Ah, thanks, I didn't realize you had opened the second PR already. Because install is effectively broken on *nix right now, do we want to merge this as a temporary fix? (I'm thinking it's find to wait considering I seem to be the first person to notice, unless there's an issue I missed.)

alef added a commit to alef/Cataclysm-DDA that referenced this pull request Nov 16, 2022
alef added a commit to alef/Cataclysm-DDA that referenced this pull request Nov 25, 2022
@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. Please do not bump or comment on this issue unless you are actively working on it. Stale issues, and stale issues that are closed are still considered.

@github-actions github-actions bot added the stale Closed for lack of activity, but still valid. label Dec 15, 2022
@Night-Pryanik
Copy link
Contributor

@perryprog could you please resolve conflicts?

@github-actions github-actions bot removed the stale Closed for lack of activity, but still valid. label Dec 16, 2022
@perryprog
Copy link
Contributor Author

Oops, sorry—been busy with real life so I haven't had a chance to look at this PR. I'll double check that this is still an issue with the merge of #61880, and if so rebase off that.

@perryprog
Copy link
Contributor Author

All done; sorry for the delay. @alef, can you confirm this still installs as expected for Windows? It looks good to me on macOS.

@perryprog perryprog mentioned this pull request Dec 23, 2022
@dseguin dseguin merged commit 9a39e7f into CleverRaven:master Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions Code: Build Issues regarding different builds and build environments json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants