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.

Sign up for GitHub

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

Jump to bottom

wayshot 1.3.0 -> 1.3.1 #302017

Merged
merged 2 commits into from
Apr 18, 2024
Merged

wayshot 1.3.0 -> 1.3.1 #302017

merged 2 commits into from
Apr 18, 2024

Conversation

id3v1669
Copy link
Contributor

@id3v1669 id3v1669 commented Apr 6, 2024

Description of changes

https://github.com/waycrate/wayshot/releases/tag/1.3.1

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.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
  • Fits CONTRIBUTING.md.

Notes

Release version of package has faulty tests with cargo test but runs as intended, so doCheck = false; was added.

Add a 👍 reaction to pull requests you find important.

@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Apr 6, 2024
CyberShadow
CyberShadow reviewed Apr 6, 2024
View reviewed changes
pkgs/tools/misc/wayshot/default.nix Outdated
cargoHash = "sha256-Hfgr+wWC5zUdHhFMwOBt57h2r94OpdJ1MQpckhYgKQQ=";
cargoHash = "sha256-K7E4od+YGb9PgmIzWSQh+sIU8kaJviftNhFpUD9vOIc=";

# added due to faulty tests in latest version
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an upstream bug report?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did not find any report in source related to this issue.
In developers makefile they do not run cargo test, just cargo build. So ignoring checks should not be an issue as they passed it to thier release version.

Copy link
Contributor

Choose a reason for hiding this comment

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

The upstream developers may appreciate a bug report about this, then.

It would also give us something to link to, so that we know what to do about disabling tests in the future. (E.g. if upstream fixes it, we can remove it; if upstream decides they don't care about tests, we will now know that.)

Copy link
Contributor Author

@id3v1669 id3v1669 Apr 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Opened issue. What about pull request to nixpkgs? Should just wait for the upstream developers' response to the issue or do something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

What about pull request to nixpkgs? Should just wait for the upstream developers' response to the issue or do something else?

No need to wait, just add a link to the issue in a comment, that explains why tests are disabled.

@ofborg ofborg bot requested a review from dit7ya April 6, 2024 06:20
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Apr 6, 2024
@id3v1669
Copy link
Contributor Author

id3v1669 commented Apr 6, 2024 •
edited
Loading

Tests are disabled for now due to fail of cargo test, will be fixed after pr #114 gets approved.
Issue: #113

@SuperSandro2000
Copy link
Member

We can fetchpatch the fix

@id3v1669
Copy link
Contributor Author

id3v1669 commented Apr 6, 2024

We can fetchpatch the fix

If you mean fetchpatch from commit. That won't be an option as commit has more fixes than version 1.3.1 needs.
Local patch can be created, but is it necessary just for one version?

@teto
Copy link
Member

teto commented Apr 6, 2024

Result of nixpkgs-review pr 302017 run on x86_64-linux 1

1 package failed to build:
  • wayshot

@id3v1669
Copy link
Contributor Author

id3v1669 commented Apr 6, 2024

Result of nixpkgs-review pr 302017 run on x86_64-linux 1

1 package built:
  • wayshot

@id3v1669
Copy link
Contributor Author

id3v1669 commented Apr 6, 2024

Result of nixpkgs-review pr 302017 run on x86_64-linux 1
1 package failed to build:

* wayshot

Just ran nixpkgs-review pr --post-result 302017, no issues.
Any ideas why that might happen?

@teto
Copy link
Member

teto commented Apr 6, 2024

@ofborg build wayshot

@id3v1669
Copy link
Contributor Author

id3v1669 commented Apr 6, 2024

@ofborg build wayshot

Looks like now it shows different cargoHash from what was before. Please try again, I've changed to the one from ofborg logs.

@id3v1669
Copy link
Contributor Author

id3v1669 commented Apr 6, 2024

@ofborg build wayshot

Ma27
Ma27 previously requested changes Apr 7, 2024
View reviewed changes
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

Package looks good functionality-wise.

Please squash your commits together into the following two chunks:

  • maintainers: add id3v1669
  • wayshot: 1.3.0 -> 1.3.1

Also, you can pass arguments to cargo test via checkFlags = [ ... ];. Maybe that can be used to skip only the tests that are actually broken?

@id3v1669
Copy link
Contributor Author

id3v1669 commented Apr 8, 2024

Package looks good functionality-wise.

Please squash your commits together into the following two chunks:

* `maintainers: add id3v1669`

* `wayshot: 1.3.0 -> 1.3.1`

Also, you can pass arguments to cargo test via checkFlags = [ ... ];. Maybe that can be used to skip only the tests that are actually broken?

Done.
Regarding testing, I would agree with you if developers actually used some of those tests, but right now, they are irrelevant for this release.
Check please this message, pr for integration testing is still WIP.

@id3v1669 id3v1669 requested a review from Ma27 April 8, 2024 06:57
@Aleksanaa Aleksanaa merged commit 9ae064b into NixOS:master Apr 18, 2024
25 checks passed
@id3v1669 id3v1669 deleted the wayshot branch April 18, 2024 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@CyberShadow CyberShadow CyberShadow left review comments

@Aleksanaa Aleksanaa Aleksanaa approved these changes

@dit7ya dit7ya Awaiting requested review from dit7ya

@Ma27 Ma27 Awaiting requested review from Ma27

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@id3v1669 @SuperSandro2000 @teto @CyberShadow @Ma27 @Aleksanaa @NixOSInfra