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

Glaxnimate: init at 0.5.1 #209669

Merged
merged 11 commits into from
Jan 9, 2023
Merged

Glaxnimate: init at 0.5.1 #209669

merged 11 commits into from
Jan 9, 2023

Conversation

tobiasBora
Copy link
Contributor

Description of changes

Fix #208707 (comment)

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@tobiasBora tobiasBora mentioned this pull request Jan 8, 2023
Co-authored-by: Weijia Wang <9713184+wegank@users.noreply.github.com>
@turion
Copy link
Contributor

turion commented Jan 8, 2023

I can build it and create a basic animation.

Working

  • Creating a basic animation
  • SVG export
  • Lottie export
  • MP4 export

Failing

  • GIF export: "2023-01-08T12:42:47" "Error" "Animated Raster" "" "Plugin raised an exception"

I'd be fine with accepting this (as soon as whitespace issues are fixed) and opening the failing GIF export as a separate issue.

tobiasBora and others added 2 commits January 8, 2023 12:46
Co-authored-by: Weijia Wang <9713184+wegank@users.noreply.github.com>
Co-authored-by: Weijia Wang <9713184+wegank@users.noreply.github.com>
@tobiasBora
Copy link
Contributor Author

tobiasBora commented Jan 8, 2023

Thanks a lot! I'll investigate the gif issue. Have you tried features requiring python?

@turion
Copy link
Contributor

turion commented Jan 8, 2023

Some simple ideas for an integration test in nixos/tests:

  • Check the output of ${glaxnimate}/bin/glaxnimate --version
  • Start glaxnimate and then use machine.wait_for_window and machine.wait_for_text to ensure the main window comes up

@turion
Copy link
Contributor

turion commented Jan 8, 2023

(But an integration test wouldn't be strictly necessary.)

@turion
Copy link
Contributor

turion commented Jan 8, 2023

Have you tried features requiring python?

No, I'm not sure which these are.

@tobiasBora
Copy link
Contributor Author

tobiasBora commented Jan 8, 2023

No, I'm not sure which these are.

Python seems to be useful for scripting and provides a console + python library. I might give it a try when I have more time in the coming week https://glaxnimate.mattbas.org/contributing/scripting/

@turion
Copy link
Contributor

turion commented Jan 8, 2023

and provides a console + python library.

For that, a separate package under the python namespace would be useful. (But not necessary for this PR)

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/kdenlive-missing-mlt-module-glaxnimate/24486/4

Co-authored-by: tricktron <tgagnaux@gmail.com>
@tricktron
Copy link
Member

tricktron commented Jan 8, 2023

Some simple ideas for an integration test in nixos/tests:

  • Check the output of ${glaxnimate}/bin/glaxnimate --version

I tried to add this test with:

passthru.tests.version = testers.testVersion {
    package = glaxnimate;
    command = "glaxnimate --version";
  };

However, on aarch64-darwin running this with nix-build -A glaxnimate.passthru.tests.version fails with glaxnimate --version returned a non-zero exit code.

On the other hand, building glaxnimate and the running manually result/bin/glaxnimate --version works.

Edit: And echo $? afterwards returns 0 as expected.

Any ideas?

@turion
Copy link
Contributor

turion commented Jan 8, 2023

I have no system to reproduce right now, but you could push a commit containing the test and we can look at the ofborg output.

Copy link
Member

@wegank wegank left a comment

Choose a reason for hiding this comment

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

Tested on aarch64-darwin.

Copy link
Member

@tricktron tricktron left a comment

Choose a reason for hiding this comment

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

Use glaxnimate --version as integration test. However, this currently fails on aarch64-darwin and possibly on linux too. If you add this, we can look at ofborg to investigate the issue.

tobiasBora and others added 2 commits January 8, 2023 21:26
Co-authored-by: tricktron <tgagnaux@gmail.com>
Co-authored-by: tricktron <tgagnaux@gmail.com>
@tobiasBora
Copy link
Contributor Author

Thanks. I think I understand the issue for Gif export, PIL is missing from python, with possibly other libraries. I'll investigate tomorrow.

@tricktron
Copy link
Member

tricktron commented Jan 8, 2023

@ofborg build glaxnimate glaxnimate.passthru.tests.version

@tobiasBora
Copy link
Contributor Author

The plugins & gif export issues are solved… the problem of glaxnimate.passthru.tests.version is that it tries to start the display, and seems like it's not available in these tests.

glaxnimate --version returned a non-zero exit code.
qt.qpa.xcb: could not connect to display 
qt.qpa.plugin: Could not load the Qt platform plugin "xcb" in "" even though it was found.
This application failed to start because no Qt platform plugin could be initialized. Reinstalling the application may fix this problem.

Not sure if there is a way to provide a display in that case.

@tobiasBora
Copy link
Contributor Author

Ok, I fixed the test script using xvfb-run. Hopefully everything should work now, feel free to check a last time so that we can merge it.

@turion turion merged commit 4695054 into NixOS:master Jan 9, 2023
@turion
Copy link
Contributor

turion commented Jan 9, 2023

Awesome, thanks a lot!

@winterqt
Copy link
Member

winterqt commented Jan 9, 2023

@turion In the case where fixes are done in separate commits, can you please squash and merge next time? 10 out of these 11 commits are just cluttering up the history now 🙃 thanks! (Luckily it's not too big of a deal since you rebased them, so I think that should avoid them being spread out...)

@turion
Copy link
Contributor

turion commented Jan 9, 2023

Sorry, I forgot to check the commit history! I'll try and remember the next times.

@tobiasBora
Copy link
Contributor Author

Yeah, I was expecting the squash to be done at merge time, hence the dirty history, I should maybe add a warning next time to remind to squash.

@tricktron
Copy link
Member

tricktron commented Jan 9, 2023

@turion @tobiasBora
xvbf is not supported on darwin. So running nix-build -A glaxnimate.passthru.tests.version will always fail on darwin with:

Package ‘xvfb-run’ in /Users/tricktron/github/my-forks/nixpkgs/pkgs/tools/misc/xvfb-run/default.nix:59 is not supported on ‘aarch64-darwin’, refusing to evaluate.

I don't know any replacement for xvfb-run on darwin and since the testers.testVersion function is mostly intended for binaries without any GUI I think it is best to just disable the passthru test on darwin.

Maybe something like:

passthru.tests.version = lib.optionalAttrs stdenv.isLinux (testers.testVersion {
    package = glaxnimate;
    command = "${xvfb-run}/bin/xvfb-run glaxnimate --version";
  });

Edit: I could open a pr if you like.

@tobiasBora
Copy link
Contributor Author

Oh good to know. Sure, feel free to write a PR !

@turion
Copy link
Contributor

turion commented Jan 9, 2023

Weird, all the CI checks were green. Aren't the tests run automatically?

@winterqt
Copy link
Member

Weird, all the CI checks were green. Aren't the tests run automatically?

@turion Normally, yes. But in this case, the commit message is wrong (should be lowercase to match the attr name, which is how OfBorg knows what to build automatically), so no. I'm admittedly not sure if it builds if more commits are added on top of that that don't include that name, though.

@turion
Copy link
Contributor

turion commented Jan 10, 2023

Ok, lesson certainly learned: Inspect commit messages closely before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

glaxnimate
7 participants