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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

nf-core: init at 2.12.1 #286496

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

nf-core: init at 2.12.1 #286496

wants to merge 12 commits into from

Conversation

edmundmiller
Copy link
Contributor

@edmundmiller edmundmiller commented Feb 5, 2024

Description of changes

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.

Add a 馃憤 reaction to pull requests you find important.

Copy link
Contributor

@zeuner zeuner left a comment

Choose a reason for hiding this comment

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

Some thoughts.

edmundmiller and others added 5 commits February 16, 2024 12:51
Co-authored-by: zeuner <zeuner@users.noreply.github.com>
Co-authored-by: zeuner <zeuner@users.noreply.github.com>
Co-authored-by: zeuner <zeuner@users.noreply.github.com>
Co-authored-by: zeuner <zeuner@users.noreply.github.com>
Co-authored-by: zeuner <zeuner@users.noreply.github.com>
@edmundmiller
Copy link
Contributor Author

@zeuner Thanks! Addressed those issues.

@rollf
Copy link
Contributor

rollf commented Apr 23, 2024

Hey @edmundmiller
thanks for your work here. I tried to run nf-core create and run into this error:

image

I checked the code and this is likely simply due to the fact the the nix-based version will not have the "right" permissions in the store. But this code here assumes this.

Using the code snippet below (with pkgs.nf-core.overrideAttrs()), I was able to get nf-core create to run through. I don't think this is the way this should be done in the end but I nevertheless wanted to share this with you.

    postPatch = ''
      file=nf_core/create.py
      for line in "# Mirror file permissions" "template_stat = os.stat(template_fn_path)" "os.chmod(output_path, template_stat.st_mode)"; do
        sed -i "/$line/d" $file
      done
    '';

I'm not sure what a clean solution would be in the end. Probably not using sed, possibly a patch. But then also, I'm not sure whether it is problematic that the "true" file permissions are not available anymore.

For testing:

nf-core create --name workflow --author "" --description ""

@edmundmiller
Copy link
Contributor Author

Thanks for testing that! Interesting solution as well.

I think that's a fine patch, maybe we can fix it on the nf-core side in future releases, and just check for permissions

@rollf
Copy link
Contributor

rollf commented May 3, 2024

@edmundmiller @zeuner I opened this PR to included nf-test. I would have added you as reviewers but I can't (I only seem to get suggestions for github handles starting with numbers and a).

@edmundmiller
Copy link
Contributor Author

@edmundmiller @zeuner I opened this PR to included nf-test. I would have added you as reviewers but I can't (I only seem to get suggestions for github handles starting with numbers and a).

Awesome!

Aside: How'd you get it merged so fast? 馃槅 #286701 has been sitting there for months.

@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-in-distress/3604/94

Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

You should squash the commits in such a way that every package gets a single init commit, and the order of commits is such that Nixpkgs is able to evaluate correctly after every commit.

Other then that, looks good to me.

@rollf
Copy link
Contributor

rollf commented May 13, 2024

@edmundmiller Based on your PR, I currently use version 2.13.1 of nf-core and have updated my permission fix. Maybe you could integrate this? (Since this is a version bump, maybe a follow-up PR would be better ?). Compared to 2.12.1 the pipeline template has greatly changed.

  nf-core = pkgs-nf-core-2.nf-core.overrideAttrs (finalAttrs: previousAttrs: rec { # pkgs-nf-core-2 points to this PR
    version = "2.13.1";

    src = fetchFromGitHub {
      owner = "nf-core";
      repo = "tools";
      rev = version;
      hash = "sha256-Wda1dmc7FDgjXIuGdLFqlkuvMU+jUVzoEh2SDWGfOAY=";
    };

    postPatch = ''
      # https://github.com/nf-core/tools/blob/master/nf_core/create.py#L345-L347
      sed -i "/# Mirror file permissions/,+2d" nf_core/create.py
      # https://github.com/nf-core/tools/blob/9eac5e9b33a4b86a5228e950a8c8fd16765f4a85/nf_core/components/create.py#L285-L289
      sed -i "/# Mirror file permissions/,+4d" nf_core/components/create.py
    '';
  });

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.

None yet

6 participants