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

Tweak to pmSDL.hpp and .gitignore #750

Closed

Conversation

serjykalstryke
Copy link
Contributor

I was having trouble building the library in totality due to some errors in the projectm-test-ui project. I was able to trace the errors to the includes in the file pmSDL.hpp. By tweaking the order to go in the order at which matches the call stack (this is I think the source of the issue. D needs C needs B needs A, so I set it to A, B, C, D, if that makes sense)

I also used vcpkg to install dependencies, so I added the ./vcpkg-install directory to .gitignore so that people who also use vcpkg don't accidentally push their dependencies to their own or the main repo.

--David Stinnett

Copy link
Member

@kblaschke kblaschke left a comment

Choose a reason for hiding this comment

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

Please squash your commits into one (or two, one per file changed, if you like) to keep the history readable and people not accidentally checking out a state that's not "good". You can use git's interactive rebase for that, then force-push the new commits to this branch (no need to recreate the PR again).

For future PRs, I would highly recommend using an extra branch for the specific changes you want to push upstream, and not your master branch, as it's way easier (e.g. with branch protections etc.) to clean up commits and stuff in development branches than the main/master branch.

@kblaschke
Copy link
Member

Oh, ignore the failing Emscripten build. We've just fixed it in our master. You could rebase your master branch on that as well if you like, but it's not necessary for this PR.

Fix generator expressions showing up in pkgconfig files

Instead of using these expressions, adding "lib" in fron of static libs is now done via CMAKE_STATIC_LIBRARY_PREFIX when building static libs on Windows.

changed order of includes in projectM-Test-UI so that it could be built

removed git modules file

deleted frontend-qt folder

made changes to pmSDL.hpp in order to fix build errors related to order of included windows libraries. Also added line in .gitignore to avoid pushing dependencies installed via vcpkg

added original .gitmodules file back, removing submodules

added original .gitmodules file back, removing submodules
@serjykalstryke
Copy link
Contributor Author

Alright, all the commits are squashed down into one and then I force pushed to update.

Copy link
Contributor Author

@serjykalstryke serjykalstryke left a comment

Choose a reason for hiding this comment

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

reviewed and changed

@kblaschke
Copy link
Member

Something seems to have gone wrong, there's now an additional merge commit in the opposite direction. To pull in upstream changes, I'd highly recommend always using git pull --rebase instead of the default merge operation.

In addition to that, the .gitmodules file was renamed to .gitmodules.txt, which would cause the submodules to fail.

If it's okay for you, I'd cherry-pick your changes into an extra branch and then create a PR for that one, even if that means eventually opening a third one. You can then simply reset your fork's master branch on upstream again to sync it without getting any merge commits or conflicts.

@kblaschke
Copy link
Member

Cherry-picked and signed your commits into a new branch and created a new PR for this one: #753
Closing this here.

@kblaschke kblaschke closed this Dec 29, 2023
@serjykalstryke
Copy link
Contributor Author

Ah yeah, ultimately that is fine with me, since I couldn’t do the merge anyways.

I still get credit for the contribution right? /j

@kblaschke
Copy link
Member

Sure, the commits still bear your author credentials, I made sure to add these when committing. Git shows this as "authored by serjykalstryke, committed by kblaschke", as there are separate fields for this.

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.

2 participants