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

Add fallback for external Qt in CMake file on Windows #166

Merged
merged 1 commit into from
Jul 7, 2024

Conversation

jahorta
Copy link
Contributor

@jahorta jahorta commented Jul 6, 2024

On a clean build using CMake on Windows without Qt6 installed, CMakeLists does not use the external library for Qt 6.5.3 provided with the source. On windows, this PR adds a quiet check for Qt6, then if it is not available it adds the external library to the prefix path list and checks again for Qt6.

Source/CMakeLists.txt Outdated Show resolved Hide resolved
@cristian64
Copy link
Collaborator

The title of the PR may be a bit too vague. Suggestion:

Add fallback for external Qt in CMake file on Windows.

@jahorta jahorta changed the title Fix cmakelists for windows Add fallback for external Qt in CMake file on Windows Jul 6, 2024
@dreamsyntax
Copy link
Collaborator

Hi, rather than have merging commit please rebase this into one commit.
We are not as strict as dolphin but please follow the general commit guidelines.

You can do
git rebase <branch>
Or at this point it may be easier to git cherry-pick with your last commit on master.

Source/CMakeLists.txt Outdated Show resolved Hide resolved
Source/CMakeLists.txt Outdated Show resolved Hide resolved
@cristian64
Copy link
Collaborator

Hi, rather than have merging commit please rebase this into one commit. We are not as strict as dolphin but please follow the general commit guidelines.

You can do git rebase <branch> Or at this point it may be easier to git cherry-pick with your last commit on master.

I guess we can ultimately "squash and merge" to keep it in a single commit. Unless the author wants to tidy things up, which is generally my preference.

@jahorta
Copy link
Contributor Author

jahorta commented Jul 6, 2024

I'm sorry. I don't really understand how git works very well and I think I am just making it worse somehow... I tried to rebase, but it just added three more commits...

@dreamsyntax
Copy link
Collaborator

I'm sorry. I don't really understand how git works very well and I think I am just making it worse somehow... I tried to rebase, but it just added three more commits...

Rebasing would involve a force push. Guessing from the above you may have done it correctly but then tried to push, got the rejection advising to pull.

Instead do a rebase then force push
(git push -f)

@jahorta
Copy link
Contributor Author

jahorta commented Jul 7, 2024

Okay, I was able to do the force-push, but now I don't know why there is an extra commit (Update status icon with Dolphin's logo redesign). Not sure why it is showing up here since I forked the repository after this was committed...

@dreamsyntax
Copy link
Collaborator

dreamsyntax commented Jul 7, 2024

You will need to rebase on the latest master to get around this.

If your master is not up to date, GitHub has a button to sync it.
Alternatively, you can add another remote and reference https://github.com/aldelaro5/dolphin-memory-engine/ directly instead, something like 'upstream' perhaps.
https://stackoverflow.com/questions/8948803/what-does-git-remote-add-upstream-help-achieve

You can:
git checkout master
git pull
git checkout - (the - brings you to prior branch)
git rebase master
git push -f

@dreamsyntax
Copy link
Collaborator

You may want to just start off with a fresh branch at this point and cherry pick your commits:
git cherry-pick 6b1bab6097111b918200afaacfc6dbd5469a3f55
etc

@cristian64
Copy link
Collaborator

Note that you do not need to push to the remote until you have verified locally that the branch is based on the master branch and that it has a single commit with the expected changes.

@jahorta
Copy link
Contributor Author

jahorta commented Jul 7, 2024

Maybe I misunderstood. Should I have all my changes in a single commit?

@dreamsyntax
Copy link
Collaborator

Maybe I misunderstood. Should I have all my changes in a single commit?

In this case, yes. Since the changes are just updating to meet the repo standards. When working on a big feature it may make sense to have multiple commits. IMO for this we should just have one, with all your updates.

Your current push looks good to me, usually 1 commit is better but in this case its fine to use GH squash feature imo.

If you want to make your three commits one, you can squash them locally like this:

You can do:
git rebase -i HEAD~3

Changing the two latter commits from
pick to squash (or just letter s works)

Using this command will put you into vi/vim by default.
If you are not familiar here's a video with explanations and timestamps:
https://youtu.be/ggSyF1SVFr4


That said, what you have right now is fine. We can just use GitHub's squash button feature when merging. Purely just posting the above for learning purposes.

@cristian64
Copy link
Collaborator

Maybe I misunderstood. Should I have all my changes in a single commit?

I think it's fine to have more than one commit in a branch. Commits need to be well polished with a properly written title and summary.

In this PR, I don't think we'd expect any more than one single commit with only changes in the CMake file.

@dreamsyntax
Copy link
Collaborator

Thanks for working on this, I hope we didn't overwhelm you with the responses. Just trying to help since these skills can be useful for contributing to other software in the future :)

@jahorta
Copy link
Contributor Author

jahorta commented Jul 7, 2024

Okay, let me try to combine them then.

Source/dolphin-memory-engine.vcxproj Outdated Show resolved Hide resolved
Copy link
Collaborator

@dreamsyntax dreamsyntax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified.

Tested:

Given Windows 11 23H2 with Visual Studio, C++ Tools & Windows SDK installed, no Qt installed on system, and the vcxproj file deleted:

  • cloning this PR and NOT pulling any submodules, then running cmake locally correctly fails with:
CMake Error at CMakeLists.txt:56 (find_package):
  By not providing "FindQt6Widgets.cmake" in CMAKE_MODULE_PATH this project
  has asked CMake to find a package configuration file provided by
  "Qt6Widgets", but CMake did not find one.

  Could not find a package configuration file provided by "Qt6Widgets" with
  any of the following names:

    Qt6WidgetsConfig.cmake
    qt6widgets-config.cmake

  Add the installation prefix of "Qt6Widgets" to CMAKE_PREFIX_PATH or set
  "Qt6Widgets_DIR" to a directory containing one of the above files.  If
  "Qt6Widgets" provides a separate development package or SDK, be sure it has
  been installed.


-- Configuring incomplete, errors occurred!
  • pulling the Windows Qt submodule, then re-running the command successfully builds using the external as expected.

Thanks for doing this.
@cristian64 feel free to merge unless you have any concerns.

@cristian64 cristian64 merged commit 277ff8d into aldelaro5:master Jul 7, 2024
4 checks passed
@jahorta jahorta deleted the fix_cmake_for_windows branch July 7, 2024 05:33
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.

3 participants