-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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
zrythm: 1.0.0-beta.4.9.1 -> 1.0.0-rc.1 #306959
Conversation
(people involved in previous PRs mentioned) |
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.
Just a few changes requested. Also, since there were so many versions of this PR, it would probably make sense to add them all as Co-authors so it's documented.
thanks for the review, will make these changes once the tar.gz arrives for rc.1 |
Upstream maintainer here, thanks for packaging this. Some feedback below. These can/should be removed: These should be moved to native build input ("make" dependencies): |
@alex-tee libxml2 is still required as a native input for xmllint, and the cmake still lists sdl2, serd, sord, and sratom as runtime dependencies |
"the cmake"? |
@alex-tee ah apologies, i misinterpreted the logs. it's the meson build file: here is a screenshot |
serd/sord/sratom/lilv are only needed at build time (lilv is used in tests), regardless if that says "runtime dependencies". They are used as "runtime dependencies" by a tool that gets compiled and used during tests, but they are not linked into the main Zrythm binary. Regarding SDL, it gets marked as required because of Please remove that as the SDL backend is broken and experimental. |
superfluous dependencies have been removed! |
LGTM, thanks! |
Thanks for taking a look at this @alex-tee! |
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.
Looks good! Just one comment. Not sure how the manual should be added, if it's something we want to add to the output. Could be worth it for completeness given that v1 is on its way, but also I don't know how the output is supposed to be used. I assume it's equivalent to the live version at https://docs.zrythm.org/.
@@ -197,7 +174,6 @@ stdenv.mkDerivation rec { | |||
"-Dmanpage=true" | |||
"-Drtaudio=enabled" | |||
"-Drtmidi=enabled" | |||
"-Dsdl=enabled" | |||
# "-Duser_manual=true" # needs sphinx-intl |
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.
I thought this comment was interesting, so I decided to look into it.
git blame
shows that it's over 3 years old! Since then, python312Packages.sphinx-intl
has been added to nixpkgs. I don't know how important this is to people, but given that we have this package now I think this could be enabled. Not sure how important this is to people.
Thoughts?
cc @tshaynik
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.
I don't know what the nix packager way to do is, but, for reference, the python requirements to build the user manual are in requirements.txt
. This is how you can build the user manual in HTML format in English:
python3 -m venv venv
source ./venv/bin/activate
# Note: sphinx and friends need to be in the PATH before configuring meson so that user manual-related ninja targets are created
python3 -m pip install -r requirements.txt
# setup meson (meson setup builddir ...)
ninja -C builddir html-manual-en
Then the built manual will be available in the builddir under doc/user.
@PowerUser64 |
8aa4bb0
to
ebfac48
Compare
Co-authored-by: PowerUser64 <blake@blakenorth.net> Co-authored-by: davidak <git@davidak.de>
I've marked this as ready for review now as rtaudio_6 has been merged. Adding the user manual could be added in another PR |
Nice work @Astavie! Glad to finally get this merged! |
The distinction between
Then |
Description of changes
Update zrythm to v1.0.0-rc.1.
This depends on #291339 as zrythm depends on gtk 4.14. This is why this PR targets
staging-next
.Closes #252561
Closes #253765
Closes #290598
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.