Skip to content

urbit: 0.7.3 -> 1.15#208483

Closed
oneingan wants to merge 13 commits intoNixOS:masterfrom
oneingan:urbit
Closed

urbit: 0.7.3 -> 1.15#208483
oneingan wants to merge 13 commits intoNixOS:masterfrom
oneingan:urbit

Conversation

@oneingan
Copy link
Copy Markdown
Contributor

@oneingan oneingan commented Dec 31, 2022

Description of changes

Bump up urbit binary, reusing own urbit nix build process code. Almost untouched.

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.

@nixos-discourse
Copy link
Copy Markdown

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/1631

Copy link
Copy Markdown
Member

@AndersonTorres AndersonTorres left a comment

Choose a reason for hiding this comment

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

17 changed files and you call this "almost untouched"??

@oneingan
Copy link
Copy Markdown
Contributor Author

I mean almost untouched from upstream (urbit build system)

@AndersonTorres
Copy link
Copy Markdown
Member

I mean almost untouched from upstream (urbit build system)

It is not because urbit uses Nix as build tool that we should copy-paste the Nix files they are using. Using the same tool does not imply to achieve the same goal.

No one in pkgsrc project copy-pastes Makefiles merely because pkgsrc is made of Makefiles.

@oneingan
Copy link
Copy Markdown
Contributor Author

Well, is not copy-paste, effectively upstream goal is much more wide: static binaries, cross build, release tarballs, debug builds, test networks and more.

I have extracted only minimal for binary build. Mainly libs ( some urbit owned, some vendored and some other override from nixpkgs).

@AndersonTorres
Copy link
Copy Markdown
Member

Well, is not copy-paste, effectively upstream goal is much more wide:

Oh, sorry!

You have copied only 5%, not the 100% of upstream git repo, so it is only a extraction, not a copy-paste!
(Even if this was almost untouched from upstream (urbit build system))

My imprecision shattered the whole critique! (or maybe only 5%, dunno...)

I have extracted only minimal for binary build

Including machine-generated files as if they were pristine sources is not "minimal".

This is low quality code.

@oneingan
Copy link
Copy Markdown
Contributor Author

oneingan commented Dec 31, 2022

Thanks for your review and your feedback. My only interest in extract "almost untouched" code was for an ease maintenance. But I understand this is more difficult to review then.

So, following your guidelines I have removed generated sources.nix code and also some dead code for static or debug builds.

Copy link
Copy Markdown
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

Please follow the contributing guide when naming your commits.

If packages are more complex and require patches then the nix file should be in the same directory as a default.nix

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why overwrite this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Darwin support and avoid beta versions. Is it preferred to send PR upstream to add Darwin support? Otoh I'll try using the beta.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've checked and h2o stable (2.2.6) is required at the moment. I could try updating lib upstream, but it will be hard to get accepted due to the beta status of version 2.3.0. Would it make sense to have both versions in nixpkgs?

@oneingan oneingan marked this pull request as draft January 4, 2023 06:54
@oneingan
Copy link
Copy Markdown
Contributor Author

oneingan commented Jan 4, 2023

Hi @SuperSandro2000, I'm wondering if it would be better to refactor this and do a Pull Request for each library:

  • libaes_siv - An RFC5297-compliant C implementation of AES-SIV
  • libent - Cross-platform wrapper around getentropy
  • murmur3 - C port of Murmur3 hash
  • softfloat3 - C implementation of binary floating-point
  • urcrypt - A library of cryptography routines used by urbit jets

or do you prefer keep them into urbit namespace?

@oneingan
Copy link
Copy Markdown
Contributor Author

Closed because build system migrate to bazel upstream

@oneingan oneingan closed this Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants