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

[devtools] Add flake.nix - INITIAL DRAFT #884

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

akavel
Copy link

@akavel akavel commented Jan 7, 2021

HACK: changed cpp/unit_tests.cc as __init__.py seemed to be throwing mypy off and I'm currently deleting it before starting the build.

@andychu
Copy link
Contributor

andychu commented Jan 7, 2021

OK I don't understand the code, but we can have it in the repo as an experiment. Thanks for taking care not to disturbe things (later we can patch the shell scripts for real)

Please link https://github.com/oilshell/oil/wiki/Developing-Oil-With-Nix and update that page as mentioned

And it would be interesting to get feedback from @abathur on the use of Nix flakes

It sounds like Nix flakes is an improvement but I want to see if people like it in practice

@abathur
Copy link
Collaborator

abathur commented Jan 8, 2021

I don't expect to have much review-y feedback on flakes in the immediate term; I'm still keeping my head in the sand on them (and they're taking longer to hit official release than I expected...) to stay focused on feature work.

I would think/hope we can find someone in the Nix community that can help field questions and review implementation.

In any case, I'm happy to field questions--I'll keep my eyes peeled :)

@akavel
Copy link
Author

akavel commented Jan 8, 2021

@abathur Good idea about asking in the Nix community! I suppose Oil might be known and respected enough that there's a chance someone might care and find it worth their time to take a look at this. Though this also probably means, that it would be best if I tried to polish up this PR to the best of my abilities, so that we can get the most value of people's limited time when someone actually comes and looks on this PR...

@andychu I have a couple more questions on this PR, that would really help me with the follow up work I need to do on it:

  1. Is the __init__.py file in the repo's main directory important and required? It seems to throw off mypy's package path resolution logic when doing it in Nix sandbox; I'd love it if you were OK with deleting this file as part of this PR. If not, the kludge that I added works for now.
  2. In order to be able to write the guide reasonably well, I'd like if I could understand a bit better some circumstances and context/background around toil-worker's run-cpp task. First of all, kinda the main question that I have on my mind all the time: given that this task seems to be only running some tests, does it mean that the C++ "path" of building an Oil/OSH binary is not yet mature enough to actually emit such a final binary? Or is it already emitted in one of the subtasks, I just didn't realize that? Or is there a different subtask that's doing this? I guess, my main question here is, who's the target audience that I'm gonna be writing the guide for? I.e., people doing what in the Oil repo? Given that this Nix expression (how they call .nix files) runs only a subset of processes described in the repo, what activities (in the Oil repo) can this be useful for?

Thanks!

@akavel
Copy link
Author

akavel commented Jan 8, 2021

Hm; I added a GitHub Action specification; on my fork of the repo it shows up like this:

https://github.com/akavel/oil/runs/1669647819?check_suite_focus=true

but for reasons unbeknownst to me, nothing similar that I can see seems to show up in main Oil repo's "Actions" tab :/ is it because I marked the PR as Draft (but in my repo actions seem to trigger even for branches)? or because something needs to be explicitly enabled in the repo? or because GH Actions must be added by repo owner? no idea...

edit: tested changing to "Ready for review" but doesn't seem to trigger a build

@akavel akavel marked this pull request as ready for review January 8, 2021 16:08
@akavel akavel marked this pull request as draft January 8, 2021 16:08
@abathur
Copy link
Collaborator

abathur commented Jan 8, 2021

@abathur Good idea about asking in the Nix community! I suppose Oil might be known and respected enough that there's a chance someone might care and find it worth their time to take a look at this.

There are at least a few people playing around with oil/osh. I'm not sure if it overlaps with people who are already on top of flakes or not.

Though this also probably means, that it would be best if I tried to polish up this PR to the best of my abilities, so that we can get the most value of people's limited time when someone actually comes and looks on this PR...

❤️ I wouldn't go fish for someone until you have questions or say you're ready. No rush.

  1. Is the __init__.py file in the repo's main directory important and required? It seems to throw off mypy's package path resolution logic when doing it in Nix sandbox; I'd love it if you were OK with deleting this file as part of this PR. If not, the kludge that I added works for now.

This may not be relevant--I always have a little trouble keeping python's packaging/module patterns straight in my head, and I'm not familiar with mypy--but it might overlap with a pain point I've intended to bring up eventually (don't want it distracting from higher-priority stuff), which is roughly about whether Oil's Python code can be more formally packaged (and namespaced) for others to build on.

To work around this in resholve, I patch in a setup.py and hacked together a weird shim that lets resholve refer to oil's modules as oil.x, but allows the modules themselves to reference each other without the namespace prefix.

@abathur
Copy link
Collaborator

abathur commented Jan 8, 2021

but for reasons unbeknownst to me, nothing similar that I can see seems to show up in main Oil repo's "Actions" tab :/ is it because I marked the PR as Draft (but in my repo actions seem to trigger even for branches)? or because something needs to be explicitly enabled in the repo? or because GH Actions must be added by repo owner? no idea...

I don't think it will run for a repo until the workflow is in that repo.

@andychu
Copy link
Contributor

andychu commented Jan 8, 2021

Is the init.py file in the repo's main directory important and required? It seems to throw off mypy's package path resolution logic when doing it in Nix sandbox; I'd love it if you were OK with deleting this file as part of this PR. If not, the kludge that I added works for now.

I'm pretty sure a bunch of imports will break without it, that's just how Python works. A directory isn't a package unless __init__.py is present.

I don't get why it would be a problem within Nix but not outside Nix? Maybe because Nix has to do something with PYTHONPATH ? But I thought we were doing things on a coarse-grained level. (I don't like when PYTHONPATH has 10 entries, one for every package, but I don't think that's a thing here?)


In order to be able to write the guide reasonably well, I'd like if I could understand a bit better some circumstances and context/background around toil-worker's run-cpp task. First of all, kinda the main question that I have on my mind all the time: given that this task seems to be only running some tests, does it mean that the C++ "path" of building an Oil/OSH binary is not yet mature enough to actually emit such a final binary? Or is it already emitted in one of the subtasks, I just didn't realize that? Or is there a different subtask that's doing this? I guess, my main question here is, who's the target audience that I'm gonna be writing the guide for? I.e., people doing what in the Oil repo? Given that this Nix expression (how they call .nix files) runs only a subset of processes described in the repo, what activities (in the Oil repo) can this be useful for?

I think a concrete way to phrase it is to build an environment where you can do:

$ _bin/osh_eval.dbg -c 'echo hi'
hi

That is, you should be able to clone Oil, then retrieve its dev dependencies. Then translate Python to C++, compile the resulting code, and then run that command.

There is also an automated one like:

build/mycpp.sh osh-eval-demo

which is run like this: http://travis-ci.oilshell.org/srht-jobs/2021-01-07__20-00-50.wwz/_tmp/toil/logs/osh-eval-demo.txt

I think there is some disconnect between using Nix to build binaries vs. Nix to build entire environments where you can run binaries... I remember having that conversation with Travis. I kind of want the latter more ...


The Python mycpp code is definitely wonky, and the worst code in the repo, mainly due to peeking into MyPy's internals ... I am taking the shortest path to making something work. I don't think it's worth packaging up; I think it's worth rewriting once this strategy actually works (dependent on the garbage collector). It's about 4300 lines of Python now so it's significant but feasible by one person.

@andychu
Copy link
Contributor

andychu commented Jan 8, 2021

I think there is some disconnect between using Nix to build binaries vs. Nix to build entire environments where you can run binaries... I remember having that conversation with Travis. I kind of want the latter more ...

To elaborate on this point, what my original intention was is to have something like "virtualenv for C" for Oil devs. I think most other languages have something like virtualenv.

"virtualenv" for C is also essentially "Linux containers", though e.g. Docker is too far on the other side of isolation (i.e. how do you put your .bashrc inside a Docker container; that's annoying as far as I remember).

And this virtualenv should BE THE SAME on the continuous build and on developer's machines. Basically to solve the "it works on my machine" problem.

If you run the tests on your machine, and they pass, and then you commit, it would be nice if it were "guaranteed" to pass on the continuous build. Right now that's not the case due to different Linux distros on dev boxes. I believe it's not the case for most open source projects either.

@akavel
Copy link
Author

akavel commented Jan 8, 2021

I don't get why [__init__.py] would be a problem within Nix but not outside Nix? Maybe because Nix has to do something with PYTHONPATH ? But I thought we were doing things on a coarse-grained level.

I also have no idea, and I must admit I'm absolutely a noob with Python. The only thing I found with regards to this, which actually led me to experiment with deleting __init__.py, was a section in mypy docs, which says:

For example, suppose we run the command mypy foo/bar/baz.py where foo/bar/__init__.py exists but foo/__init__.py does not. Then the module name assumed is bar.baz and the directory foo is added to mypy’s module search path.

This indeed behaved like this in the Nix sandbox, i.e. a prefix was appended to module paths, corresponding to the directory where the repo was extracted (because the directory contains __init__.py). Thus, running translate-ordered resulted in modules with an extra unexpected prefix in the imported modules list. To be fair, I'm surprised this doesn't happen in the Travis & sr.ht environments. I'm really curious what are the values in PYTHONPATH (when set to $MYPY_REPO by the script) and MYPYPATH (when set to $REPO_ROOT:$REPO_ROOT/native) in those cases (e.g. in EXAMPLE parse). Is there some way I could log them out? Maybe if I then compared them with what I see in Nix, this might shed some light? Hm, I guess maybe I can try to trigger Travis builds on my repo as well, with tweaked scripts dumping the contents of the variables to output - that's an idea!

As to "doing things on coarse-grained level", I'm not really sure what you mean here, esp. in context of this PR? Do you mean this PR is not coarse-grained enough in your opinion? Or something else? This got me confused.


Re: "build environment [...] _bin/osh_eval.dbg ... [...] osh-eval-demo [...]"

Oh, cool! I think that's kinda what I hoped to be able to do from the start with this whole Nix exercise. Thank you for giving me some concrete commands that I can work towards, it's really helpful to validate if I got to the end goal. Still, this may take me quite some more work & time to make them run, but I guess we're not in any kind of hurry with this PR. And getting to a place where I might be able see some real results from Oil compiled with help of Nix, sounds quite exciting to me!

As to using Nix to build binaries vs. to build environments: my rough view on this is such, that I believe in case of Nix, the first is kinda prerequisite for the second. By this I mean, that starting from building binaries has the benefit of a very strictly controlled and hermetic environment. So, if something fails to build in Nix, I see it as a signal that the Nix expression is broken, because this means our "full deterministic description of a world where the binary can be built" is wrong. So it makes little sense to try and create an "open" build environment on such Nix expression. But if this basis is correct, we can then try and mix it with user's environment to give them as much as possible of the deterministic parts, while also merging with their actual real-life user-friendly daily working environment.

"virtualenv" for C [...] If you run the tests on your machine, and they pass, and then you commit, it would be nice if it were "guaranteed" to pass on the continuous build. Right now that's not the case due to different Linux distros on dev boxes.

That's totally how I also believe Nix should work and benefit developers. That said, as far as I understand, I think it kind of has to be looked at as more like three "phases":

  1. There's a work-in-progress "build environment" (a.k.a. nix-shell). A developers works in it, and has all the dependencies brought via Nix (the "virtualenv for C") - but they also have their .bashrc, their .vimrc, their custom extra PYTHONPATH with some tools they can't live with, etc. etc. In other words, that's necessarily an "open", "non-hermetic" environment. As such, it guarantees them all necessary dependencies for starting the work, but doesn't yet guarantee that any new dependencies they introduce will be captured in it. And notably, with this openness and freedom, comes also openness and freedom to break this build environment.
  2. Then, there's the "hermetization" step to be done by a developer (by running nix build). In this step (which can be totally done on their computer), they are telling Nix: "take all the code, put it in a hermetic, isolated, fully described & controlled sandbox, and verify if you can successfully build it there". This is the "truth check", verifying if the developer didn't accidentally introduce some new external dependencies while working on the project, that they forgot/failed to capture in the Nix expression.
    • that's basically what I've been doing when building the .nix file ("Nix expression") which is the main part of this PR.
  3. If the "hermetization" step above passes, then Nix basically guarantees that the project will also build successfully in CI and on any other dev's machine if they use Nix (modulo machine architecture, OS and maybe kernel; but e.g. libc is controlled; and modulo non-determinism of the build process itself, which is quite tricky to avoid completely).

(And notably for (1) to work really smoothly, I believe we might eventually have to remove the need for the patching/sed-ing that is done in this .nix script/expression; either by somehow incorporating those patches into the repo, or by somehow removing the need for them in the .nix script. But I think it makes sense to keep that for later as of now, until more pieces are in their places.)

@andychu
Copy link
Contributor

andychu commented Jan 9, 2021

  • Hm with regard to __init__.py, I think it's going to be a rathole, so I would just leave it if it works now. I'd rather spend effort writing something better than tracking down a bug that only appears in one environment.
  • This does look coarse-grained since you're running the whole toil-worker.sh job. What I meant is that many tools mess with PYTHONPATH to point at specific packages, and they have a very long PYTHONPATH as a result. IMO that is usually bad news. It can work but it makes things slow and it also conflicts with how our build scripts work to a degree.

OK I think we're basically on the same page for the rest... I'm curious to see how it turns out!

I kinda prefer containers over the patchShebang stuff, but let's see how well it works, and how fast it is, etc. Ideally sandboxing should give some parallelism.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants