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

Add railsy at 0.1.1 #356107

Closed
wants to merge 2 commits into from
Closed

Add railsy at 0.1.1 #356107

wants to merge 2 commits into from

Conversation

mmkaram
Copy link

@mmkaram mmkaram commented Nov 15, 2024

Description of changes

added railsy package, homepage
added mmkaram to maintainer-list
added default.nix to railsy in pkgs/by-name/railsy
added required top level definitions

This is my first attempt at adding a package to nixpkgs, so please let me know if I did anything wrong

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Nov 15, 2024
@steeleduncan
Copy link
Contributor

The name of the commit adding yourself to the maintainers list should be maintainers: add mmkaram

@@ -0,0 +1,41 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes are in the wrong commit, they should be in railsy: init ...

@@ -5148,6 +5148,8 @@ with pkgs;

rainbowstream = with python3.pkgs; toPythonApplication rainbowstream;

railsy = callPackage ../by-name/ra/railsy { };
Copy link
Contributor

Choose a reason for hiding this comment

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

I could be wrong, but is this necessary with the package in /by-name/? I think you can remove this and it'd be fine

Copy link
Member

Choose a reason for hiding this comment

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

@steeleduncan You are right. Please remove this line.

src = fetchFromGitHub {
owner = "mmkaram";
repo = "railsy";
rev = "ref/tags/v${version}";
Copy link
Contributor

Choose a reason for hiding this comment

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

When I tried building this, there is no such tag in the mmkaram/railsy repo, you'd need to add it, or use a commit as the rev

@steeleduncan
Copy link
Contributor

steeleduncan commented Nov 15, 2024

By the way, it is not for me to decide, but this looks a lot like an early stage project, with very few commits or users. I think it is unlikely to be picked up to be merged into nixpkgs at this point

@cu1ch3n
Copy link
Member

cu1ch3n commented Nov 15, 2024

Hi @mmkaram! Thanks for the contribution but I would suggest you to read the pkgs/README and search for some similar PR examples to see if something can be learned or borrowed before submitting a PR. It will save our time. As you can see, there are thousands of PRs not reviewed.

  • You should NOT add include multiple random changes in one commit. You cannot have a commit saying "maintainers: add mmkaram" but you put everything unrelated there.
  • You should NOT use random commit message like "Add railsy at 0.1.1"
  • You should make sure that the package can be built at least on your machine
  • This project by yourself is still too immature and should not be merged into nixpkgs. I would suggest you submit it to NUR https://github.com/nix-community/NUR.

@mmkaram
Copy link
Author

mmkaram commented Nov 15, 2024

Hey duncan and cu1ch3n, thank you guys for the feedback! I'll get to all these action items and check out the NUR. Have a good one!

@mmkaram mmkaram closed this Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants