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 invalid CMakeSettings.json generation #4497

Merged
merged 3 commits into from
Nov 5, 2021
Merged

Conversation

qgazq
Copy link
Contributor

@qgazq qgazq commented Nov 1, 2021

I noticed that CMakeSettings.json is currently being generated with an invalid key when windows_buildenv.bat is used.
Visual Studio 19 therefore won't load it, and gives:
Failed to parse CMakeSettings.json. Bad JSON escape sequence: \=. Path 'configurations[0].variables[5].value', line 40, position 22.
The problem seems to be CMAKE_PREFIX_PATH is no longer setup in the bat file,
So the line: CALL :AddCMakeVar2CMakeSettings_JSON "CMAKE_PREFIX_PATH" "STRING" "!CMAKE_PREFIX_PATH:\=\\!"
Causes a line "value": "\=\\", in the CMakeSettings.json which isn't valid.
My fix is to add a set back into the file matching one that was removed by an earlier commit, I'm not sure if its the best solution as I'm not sure what if anything CMAKE_PREFIX_PATH is used for. It's possible we may not need it and it can be removed?

@Be-ing
Copy link
Contributor

Be-ing commented Nov 1, 2021

@JoergAtGithub can you review this?

@JoergAtGithub
Copy link
Member

I just tested this. I can confirm that Main is broken here. I didn't notice it, because the last CMAKE_PREFIX path remains set system wide, until you overwrite it. In my case, there were multiple old CMAKE_PREFIX paths concatenated. And one of them pointed to the correct files.

With this patch always the correct CMAKE_PREFIX path is set. This PR works for me as well for VisualStudio GUI builds, as for command line builds!
Thank you for fixing this!

@Holzhaus
Copy link
Member

Holzhaus commented Nov 5, 2021

Alright, then lets merge this. Thanks to both of you!

@Holzhaus Holzhaus merged commit d55011f into mixxxdj:main Nov 5, 2021
@daschuer
Copy link
Member

Hi @qgazq, thank you for your contribution.
It looks like we have missed to ask for your permission to distribute your code before merge. Can you catch that up, sign https://docs.google.com/a/mixxx.org/spreadsheet/viewform?formkey=dEpYN2NkVEFnWWQzbkFfM0ZYYUZ5X2c6MQ and comment here when done? I will then put your name to the Mixxx 2.4 contributor list in the Mixxx about box.

@qgazq
Copy link
Contributor Author

qgazq commented Jan 17, 2024

Hi. It wasn't much of a contribution, I was just getting it to build so I could debug an issue I was having, I'm not sure it's worth a mention on the about list. But happy to help. I've filled out the form for the record, I wasn't sure if you needed my real name, so thats what I've put on the form (its also on my github profile).

@daschuer
Copy link
Member

Finding an issue can take a lot of time even if the resulting fix is only one line and than do the extra effort and contribute a PR makes the difference. This deserves to be mentioned.

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

5 participants