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 ypbind-mt package #333099

Merged
merged 2 commits into from
Sep 5, 2024
Merged

add ypbind-mt package #333099

merged 2 commits into from
Sep 5, 2024

Conversation

BarrOff
Copy link
Contributor

@BarrOff BarrOff commented Aug 7, 2024

This is the first time I submit a pull request to nixpkgs.
I read and tried to adhere to the contributing guidelines on a best effort basis.
If there is still something wrong/missing or needs change I will gladly update the PR.

Description of changes

This pull request adds the package ypbind-mt which contains the ypbind binary needed for NIS clients to bind to NIS servers.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested basic functionality of all binary files (usually in ./result/bin/)

Misc

The company I work at is using NIS/YP and the necessary tools are still missing in nixpkgs.
This is intended to be the first in a series of packages needed to create usable NIS clients (yp-tools and libnss_nis will follow).
This deviation was created and successfully built/used on a NixOS machine.

A few changes to the Makefile for manpages are necessary.
I did not find a way to prevent xmllint from trying to access the network.
Maybe there is a better way than commenting out the respective lines using sed?

Best regards
BarrOff

@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Aug 7, 2024
@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Aug 7, 2024
maintainers/maintainer-list.nix Show resolved Hide resolved
pkgs/by-name/yp/ypbind-mt/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/yp/ypbind-mt/package.nix Outdated Show resolved Hide resolved

outputs = [ "out" ];

configurePhase = ''
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain your code here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without the configurePhase/installPhase settings the derivation tried to install the files to the global root / directory.
Has been fixed in my refactor commit 97c8bec (#333099)

@AndersonTorres
Copy link
Member

I did not find a way to prevent xmllint from trying to access the network.

Roughly speaking, XMLLINT requires some files (in a XSLT format) so that it knows what to do - think on it as providing a map with instructions to a human person.
If those files/maps are not locally present, then xmllint tries to download them (like a person opening the Maps applet).

Then you need to provide these XSLT files somehow. Usually Nixpkgs provides them already.

@BarrOff
Copy link
Contributor Author

BarrOff commented Aug 8, 2024

The Makefile has a hardcoded http url to a xml style sheet, see here and I see two solutions:

  1. Disable this by commenting out that code (my initial solution)
  2. Replace the http url with the manpage/docbook.xsl file from the docbook-xsl-nons package

By chance I tried the official release tarball from the repos release page and it worked without the problems I had with the fetchFromGithub code.
There the configure script automatically comments out the problematic lines.
It is also able to build the code without any custom configPhase/installationPhase settings.

I think this is the preferable solution and made a commit for this.

@AndersonTorres Do you see any remaining issues with the PR?

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Aug 9, 2024
@AndersonTorres
Copy link
Member

I think this is the preferable solution and made a commit for this.

Usually we prefer to use the pristine source code, so that we have more control over the build process.
It became more pronounced since the xzdoor incident.

But for now let's pick the tarball.

Copy link
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.

Besides, LGTM

pkgs/by-name/yp/ypbind-mt/package.nix Outdated Show resolved Hide resolved
@AndersonTorres
Copy link
Member

Now get rid of that excess of Semantic Commits and follow our commit conventions:

https://github.com/NixOS/nixpkgs/blob/master/pkgs/README.md#commit-conventions

@BarrOff
Copy link
Contributor Author

BarrOff commented Aug 13, 2024

Just to make sure I understand correctly how to proceed:

  1. I should do a rebase of all my commits to adjust the commit messages, then force push to my branch?
  2. Am I supposed to squash some commits where suitable?

@AndersonTorres
Copy link
Member

AndersonTorres commented Aug 20, 2024

Yes. Get rid of that truckload of "feat/fix/style/etc".

Only things are

  • maintainers: add <handle>
  • <pkg>: init at <version>

@BarrOff BarrOff force-pushed the feat-add-ypbind branch 2 times, most recently from 449086d to a62ec9e Compare August 21, 2024 20:59
@BarrOff
Copy link
Contributor Author

BarrOff commented Aug 21, 2024

Most projects I have worked on before use conventional commits so I used that.
In the meantime I read the contributing guide again and noticed the commit style guide.
I have rebased and squashed the commits as requested.

Copy link
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.

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

1 package built:
  • ypbind-mt

@K900 K900 merged commit cd36278 into NixOS:master Sep 5, 2024
36 checks passed
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` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 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. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants