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

openrussian-cli: fix build & misc cleanups #263916

Merged
merged 2 commits into from
Nov 16, 2023
Merged

openrussian-cli: fix build & misc cleanups #263916

merged 2 commits into from
Nov 16, 2023

Conversation

vs49688
Copy link
Contributor

@vs49688 vs49688 commented Oct 28, 2023 •
edited
Loading

Description of changes

ZHF: #265948

  • 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/)
  • 23.11 Release Notes (or backporting 23.05 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.

@vs49688
Copy link
Contributor Author

vs49688 commented Oct 28, 2023

@ofborg build openrussian-cli

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/2832

@vs49688 vs49688 added the 0.kind: ZHF Fixes Fixes during the Zero Hydra Failures (ZHF) campaign label Nov 7, 2023
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/2895

onemoresuza
onemoresuza suggested changes Nov 8, 2023
View reviewed changes
Copy link
Contributor

@onemoresuza onemoresuza left a comment

Choose a reason for hiding this comment

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

Reviewed points

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

1 package built:
  • openrussian-cli
  • package name fits guidelines
  • package version fits guidelines
  • package build on x86_64-linux
  • executables tested on x86_64-linux
  • all depending packages build

Possible Improvements

  1. Package version
    As of 4e4bbb01, there's a new naming convention for
    packages    :

    If a package is a commit from a repository without a version assigned,
    then the version attribute should be the latest upstream version preceding
    that commit, followed by -unstable- and the date of the (fetched) commit.
    The date must be in "YYYY-MM-DD" format.

@vs49688
Copy link
Contributor Author

vs49688 commented Nov 9, 2023 

1. Package version
   As of [4e4bbb01](https://github.com/NixOS/nixpkgs/blame/4e4bbb01ebfa4a1ce322f036007ec4d653071523/pkgs/README.md#L313), there's a new [naming convention for
   packages](https://github.com/NixOS/nixpkgs/blob/4e4bbb01ebfa4a1ce322f036007ec4d653071523/pkgs/README.md#package-naming):
   > If a package is a commit from a repository without a version assigned,
   > then the `version` attribute should be the latest upstream version preceding
   > that commit, followed by -`unstable`- and the date of the (fetched) commit.
   > The date must be in "`YYYY-MM-DD`" format.

Huh, I was not aware of this change. Fixed.

onemoresuza
onemoresuza approved these changes Nov 9, 2023
View reviewed changes
@vs49688 vs49688 added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 9, 2023
@vs49688 vs49688 changed the title openrussian-cli: 1.0.0 -> unstable-2023-10-06 openrussian-cli: 1.0.0 -> 1.0.0-unstable-2023-10-06 Nov 10, 2023
@pbsds
Copy link
Member

pbsds commented Nov 12, 2023 •
edited
Loading

@ofborg build openrussian-cli openrussian-cli.passthru.tests

@vs49688
Copy link
Contributor Author

vs49688 commented Nov 14, 2023

Ping on this?

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/prs-already-reviewed/2617/1230

@vs49688
Copy link
Contributor Author

vs49688 commented Nov 16, 2023

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

1 package built:
  • openrussian-cli

@pbsds
Copy link
Member

pbsds commented Nov 16, 2023

Seems like the darwin builds keep timing out. Could you mark it broken on darwin?

@pbsds
Copy link
Member

pbsds commented Nov 16, 2023 •
edited
Loading

Instead of bumping to an unstable version, could you apply rhaberkorn/openrussian-cli@984e555 as a patch? This is the only non-readme change anyway.

@vs49688
Copy link
Contributor Author

vs49688 commented Nov 16, 2023 •
edited
Loading

Instead of bumping to an unstable version, could you apply rhaberkorn/openrussian-cli@984e555 as a patch? This is the only non-readme change anyway.

Done, and I applied a few other changes too - ran it through nixpkgs-fmt and converted to pkgs/by-name.

I'm just debugging the Darwin issue now, if I can't solve it, I'll mark it as broken.

@vs49688
Copy link
Contributor Author

vs49688 commented Nov 16, 2023 •
edited
Loading

This is odd, it builds successfully on my M1 when run manually in a nix develop shell.
When run via nix build, it hangs on in the mysql2sqlite script (both with and without sandboxing enabled). Trace output:

+ Op_push_re
+ Op_push_i
+ Op_push_i
+ Op_field_spec_lhs
+ Op_sub_builtin
+ Op_field_assign
+ Op_pop
+ Op_push_re
+ Op_push_i
+ Op_push_i
+ Op_field_spec_lhs
+ Op_sub_builtin

I say hang in a loose sense - it eventually progresses to another Op_sub_builtin, and I'd image it'd finish eventually.

@vs49688
openrussian-cli: fix build & misc cleanups
7c9a659 
* Backport patch to fix DB generation after SQLite's removal of "" handling
* Format
* Convert to pkgs/by-name
* Mark broken on Darwin because the mysql2sqlite script hangs.
@vs49688
Copy link
Contributor Author

vs49688 commented Nov 16, 2023

Marked broken on Darwin for now.

@vs49688
Copy link
Contributor Author

vs49688 commented Nov 16, 2023

@ofborg build openrussian-cli

@vs49688 vs49688 changed the title openrussian-cli: 1.0.0 -> 1.0.0-unstable-2023-10-06 openrussian-cli: fix build & misc cleanups Nov 16, 2023
@vs49688
Copy link
Contributor Author

vs49688 commented Nov 16, 2023

@ofborg build openrussian-cli

@vs49688
Copy link
Contributor Author

vs49688 commented Nov 16, 2023

Darwin support fixed:

Result of nixpkgs-review pr 263916 run on aarch64-darwin 1

1 package built:
  • openrussian-cli

@pbsds pbsds merged commit b74d956 into NixOS:master Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@onemoresuza onemoresuza onemoresuza approved these changes

Assignees
No one assigned
Labels
0.kind: ZHF Fixes Fixes during the Zero Hydra Failures (ZHF) campaign 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 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.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vs49688 @nixos-discourse @pbsds @onemoresuza