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

Fix CMakeLists.txt error #209

Merged
merged 3 commits into from
Jun 20, 2023
Merged

Fix CMakeLists.txt error #209

merged 3 commits into from
Jun 20, 2023

Conversation

4G3NT
Copy link
Contributor

@4G3NT 4G3NT commented Jun 20, 2023

Don't read CMAKE_BUILD_TYPE if it's not defined in order to fix an error:

CMake Error at CMakeLists.txt:75 (string):
string no output variable specified

-- Configuring incomplete, errors occurred!

@Lymphatus Lymphatus merged commit 990ae92 into Lymphatus:main Jun 20, 2023
@4G3NT
Copy link
Contributor Author

4G3NT commented Jun 20, 2023

We can also have CMAKE_PREFIX_PATH default to a valid value, but since the cmake file spits out an understandable error, I don't think that's necessary.

@Lymphatus
Copy link
Owner

Lymphatus commented Jun 20, 2023

For CMAKE_PREFIX_PATH it's a bit harder to have a default value. It varies upon platforms and you can always customize the Qt path for your specific setup. If you give a default value and that is not correct, the error that will come out is far harder to understand than the one that's in place right now.

@4G3NT
Copy link
Contributor Author

4G3NT commented Jun 20, 2023

Yeah, good point it's really hard to set it to a good default.

@4G3NT
Copy link
Contributor Author

4G3NT commented Jun 20, 2023

Second if statement might be unnecessary too.

@Lymphatus
Copy link
Owner

Lymphatus commented Jun 20, 2023

I'm no cmake expert whatsover and the current file is a bit of a mess, result of many trials and errors in the past. It works now, but I'd like to refactor it a little bit to make it more clear and useful in the future. It also helps that now Github Actions are in place and there's an automated way to know if a script works on all platforms at once.

@4G3NT
Copy link
Contributor Author

4G3NT commented Jun 20, 2023

I will delete the if statement as it is unnecessary and I will add another fix for the INSTALL make target.

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.

None yet

2 participants