-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Store workspaces files uncompressed in the repository #4579
Store workspaces files uncompressed in the repository #4579
Conversation
Duplicate of #4341? :) |
Let's decide which one is better after the release. |
fffa632
to
398065a
Compare
Oh, I wasn't aware of that :) |
398065a
to
79348a7
Compare
I'd go for this one personally. Mine is quite out of date with all the changes that keep happening with the workspace and this one looks better. It also has more info in the pr that would be useful for any later changes/blames. |
Can we please have this sooner rather than later? There are several open issues that require changed to the workspaces, better to have this here first. |
If these are being unzipped by QZipReader then you are likely to run into this issue: https://musescore.org/en/node/280231. |
Unzipped? |
@shoogle is probably talking about unzipping the file when it is read by MuseScore. And, according to this code, it is indeed done with QZipReader. Still I didn't run into this issue while testing this PR locally. But this should be probably tested on all platforms, or it would be even better if the mentioned issue could be resolved. |
The unzipping has always been done with this method and never cause an issue as far as I know. Zipping neither. But zipping with something other than QZipReader may indeed cause issues when unzipping with that. However this is not done here as far as I can tell |
set(_FILES workspace.xml META-INF/container.xml) | ||
add_custom_command(OUTPUT ${_OUTPUT} | ||
COMMAND ${CMAKE_COMMAND} -E tar cf "${_OUTPUT}" --format=zip -- ${_FILES} | ||
DEPENDS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be DEPENDS ${_FILES}
to ensure the zip is rebuilt when the files change.
It might be necessary to prepend ${CMAKE_CURRENT_SOURCE_DIR}
to each of the file paths because the behaviour of DEPENDS
is counter-intuitive (see this code comment in PR #3269). You can use this function to prepend a string to each item in a list:
prepend_to_list_items(_FILES "${CMAKE_CURRENT_SOURCE_DIR}/" _FILES_ABS)
You will probably need to copy the function code into this file because it won't be accessible outside of the mscore
directory where it is defined.
@Jojo-Schmitz, this PR zips the files with |
Ah, now I got it. Seems to work on Windows |
This PR makes workspaces data be stored uncompressed in the repository. This makes it easier to track changes made to them in version control system. The workspace files being installed are produced at build time using CMake built-in
tar
tool. Potentially important that this change requires raising CMake minimal required version to 3.3. Please leave a comment here if this is undesirable for some reason.I updated also a license header in the changed
CMakeLists.txt
, it was apparently not very actual.