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 nix configuration file for development setup #19586

Closed
wants to merge 5 commits into from
Closed

add nix configuration file for development setup #19586

wants to merge 5 commits into from

Conversation

mberndt123
Copy link
Contributor

Hey,

after I've had a lot of trouble setting up a development environment for Pants, I'd like to contribute the kind of development setup workflow that I wish I had had.

This PR includes a nix configuration file that specifies the necessary dependencies. It also includes documentation about how to use it.
To ensure that I haven't missed any dependencies and that everything works correctly, I created a pretty minimal Ubuntu 22.04 container (I installed only git, curl, xz-utils and nix), cloned the Pants repo and bootstrapped pants – it seems to work flawlessly.

Copy link
Member

@thejcannon thejcannon left a comment

Choose a reason for hiding this comment

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

I think the comment here: #19585 (comment) is still very relevant

@mberndt123
Copy link
Contributor Author

@thejcannon

That comment is about PBS, and PBS isn't used at all in this setup. Am I missing something here?

@thejcannon
Copy link
Member

Sorry, I should've commented on the clang line.

Also, we might just rip the PBS band-aid though (from this discussion) since it looks like hdrhistogram might be getting wheels: HdrHistogram/HdrHistogram_py#47

@mberndt123
Copy link
Contributor Author

Well, even if hdrhistogram grows wheels, there's still protobuf, Python, rustup etc. that need to be installed. So I think a nix configuration still makes things easier. I'd rather install one tool than three.

@thejcannon
Copy link
Member

Yeah my point is just that clang likely shouldn't be in this list and with PBS, python will be out as well.

Then, others can decide whether we want to accept this file. I can't say I've seen many other projects using this kind of thing, or that I personally have ever used nix before, so I'm not sure I'm the best to approve it

@benjyw benjyw added the category:internal CI, fixes for not-yet-released features, etc. label Aug 10, 2023
@benjyw benjyw requested a review from jriddy August 10, 2023 22:39
@benjyw
Copy link
Contributor

benjyw commented Aug 10, 2023

I've never used nix so I can't comment on the specifics, but I am generally down with the concept! I think @jriddy is well placed to review and comment.

@mberndt123
Copy link
Contributor Author

Well, @jriddy, you could also approach it from the other side: if you have Nix, you no longer need PBS – you can just use the tool that you're going to use anyway to install the other development tools, i. e. rustup and protoc.

I suppose you just give it a try and see if it works for you 😃 Ideally on a Mac, which I don't have and therefore cannot test.

@jriddy
Copy link
Contributor

jriddy commented Aug 11, 2023

I don't have a Mac but I can test with a standalone version of Nix on Fedora or Debian.

@@ -26,6 +26,10 @@ We use the popular forking workflow typically used by open source projects. See
Step 2: Bootstrap the Rust engine
---------------------------------

Pants requires several dependencies to be installed: a Python 3.9 interpreter, Rust, the protobuf compiler, clang and others. The easiest and fastest way to set up an environment with all necessary dependencies is to use the Nix package manager. The Pants repository includes a suitable `shell.nix` file. Follow the instructions on the [Nix website](https://nixos.org/download.html) to install Nix. Then `cd` into the directory where you cloned the Pants repo and type `nix-shell`. This will download all the necessary dependencies and start a shell with a suitably configured PATH variable to make them available for use.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be wary of using wording that suggests this is a blessed way getting a dev environment up and running until we have reasonable quorum of contributors using it.

When we say "the easiest and fastest" way to do something is X, we're implicitly committing to supporting that methodology. Since almost none of us have extensive experience with Nix, I wouldn't want to claim or even suggest that.

More conservatively, we could claim there's experimental support for a Nix closure (is that the right terminology) that makes dev setup easy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, I've changed the wording to reflect this.

@huonw huonw requested a review from jriddy August 22, 2023 02:45
shell.nix Outdated Show resolved Hide resolved
@mberndt123 mberndt123 closed this by deleting the head repository Oct 22, 2023
@benjyw
Copy link
Contributor

benjyw commented Oct 23, 2023

Hi @mberndt123, did we drop the ball on review iteration on this? Apologies if so. I'd still be open to merging it. Alternatively we can consider using nix to build pants itself if/when we add support for pants using nix, per #19537

@mberndt123
Copy link
Contributor Author

Hey @benjyw,
Yes, I recently went through my repos and removed those that I no longer needed. Since you've expressed interested, I've now restored the repo – but for some reason GitHub still won't let me Reopen the PR 🤷🏻‍♂️
Anyway, I have a branch here that you can merge
https://github.com/mberndt123/pants/tree/add-nix-setup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants