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

retdec: 3.2 -> 5.0 #285950

Merged
merged 3 commits into from
Feb 5, 2024
Merged

retdec: 3.2 -> 5.0 #285950

merged 3 commits into from
Feb 5, 2024

Conversation

katrinafyi
Copy link
Member

@katrinafyi katrinafyi commented Feb 3, 2024

Description of changes

https://github.com/avast/retdec/blob/master/CHANGELOG.md

  • Bumps vendored dependencies and remove ones no longer needed.
  • Since 3.3, compiled patterns are not shipped in the support file, obviating the postFetch strip. (YARA patterns in the static-code directory are very large avast/retdec-support#3)
  • Now, patterns may be compiled at build time and an argument is provided to control this (on by default).
    • As such, retdec-full is no longer needed and removed. The 60MB increase from the compiled patterns seems more preferred than duplicating the whole 500MB size.
  • We use cmake _URL variables to insert dependencies and we are able to use nixpkgs googletest.
  • Fix build with current gcc 13.
  • Remove i686 from platforms as derivation script needs to specify lib64.

On my laptop, this package takes 30 minutes to build. Half of this is the fork of LLVM.

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.

cc maintainers: @timokau @dtzWill


Add a 👍 reaction to pull requests you find important.

Copy link
Member

@timokau timokau left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this!

I am not using this package anymore and therefore cannot maintain it. Would you be willing to become a maintainer? If so, please add yourself to the maintainers list and remove my entry.

pkgs/development/tools/analysis/retdec/default.nix Outdated Show resolved Hide resolved
@katrinafyi
Copy link
Member Author

Addressed changes (use absolute patch url and change maintainers) @timokau

I had a question, does anything else need to be done to obsolete the retdec-full package?

@katrinafyi
Copy link
Member Author

Fixed build failure due to stale hash.

@timokau
Copy link
Member

timokau commented Feb 4, 2024

Thanks again!

Regarding retdec-full, I think you should follow these steps: https://github.com/NixOS/nixpkgs/blob/master/pkgs/README.md#steps-to-remove-a-package-from-nixpkgs

name = "katrinafyi";
github = "katrinafyi";
githubId = 39479354;
};
Copy link
Member

Choose a reason for hiding this comment

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

Welcome to the maintainers team 🎉

- Bumps vendored dependencies and remove ones no longer needed.
- Since 3.3, compiled patterns are not shipped in the support file, obviating the postFetch strip. (avast/retdec-support#3)
- Now, patterns may be compiled at build time and an argument is provided to control this (on by default).
- As such, retdec-full is no longer needed and removed. The 60MB increase seems more preferred than duplicating the 500MB size.
- We use cmake _URL variables to insert dependencies and we are able to use nixpkgs googletest.
- Fix build with current gcc 13.
- Remove i686 from platforms, as derivation needs to specify lib64.
- Maintainers: remove timokau, add katrinafyi.
since version 5.0, compiled yara patterns are included with the retdec package
so retdec-full is no longer needed.
@katrinafyi
Copy link
Member Author

katrinafyi commented Feb 5, 2024

Formally removed retdec-full.

Copy link
Member

@timokau timokau left a comment

Choose a reason for hiding this comment

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

Thank you! Quite an impressive first contribution :)

@timokau timokau merged commit 80b01b1 into NixOS:master Feb 5, 2024
23 of 24 checks passed
@raspher raspher mentioned this pull request Feb 8, 2024
2 tasks
--replace-quiet VersionTests DISABLED_VersionTests

substituteInPlace scripts/retdec-utils.py \
--replace-warn /usr/bin/time ${time} \
Copy link
Member

Choose a reason for hiding this comment

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

any reason not to use --replace-fail here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really, and it would be good to fail more obviously here. I have an open PR for this package (#293962) and I can make the changes there.

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.

4 participants