Skip to content

nix-build: Port to c++#1018

Merged
shlevy merged 3 commits intoNixOS:masterfrom
shlevy:nix-build-c++
Aug 31, 2016
Merged

nix-build: Port to c++#1018
shlevy merged 3 commits intoNixOS:masterfrom
shlevy:nix-build-c++

Conversation

@shlevy
Copy link
Copy Markdown
Member

@shlevy shlevy commented Aug 9, 2016

This was a dumb line-for-line rewrite, because nix build/nix run/etc.
will replace it.

@shlevy shlevy added this to the perl-to-c++ milestone Aug 9, 2016
@shlevy shlevy force-pushed the nix-build-c++ branch 2 times, most recently from dbf1b3b to 2c38cd7 Compare August 9, 2016 11:33
This was a dumb line-for-line rewrite, because nix build/nix run/etc.
will replace it.
@shlevy shlevy mentioned this pull request Aug 9, 2016
12 tasks
vcunat added a commit to vcunat/nix that referenced this pull request Aug 11, 2016
An equivalent was originally filed against the perl version:
NixOS#1018
An equivalent was originally filed against the perl version:
NixOS#933
@vcunat
Copy link
Copy Markdown
Member

vcunat commented Aug 11, 2016

It seems some merge conflicts have originated in the meantime.

@shlevy
Copy link
Copy Markdown
Member Author

shlevy commented Aug 11, 2016

Yep, I'll merge + fix conflicts once I get an OK from @edolstra

@shlevy shlevy mentioned this pull request Aug 12, 2016
@hedning
Copy link
Copy Markdown

hedning commented Aug 16, 2016

There's a bug, #976, with nix-shell due to $BASH_ENV being set. Seems like this code also does this. See #1034 for a fix of the perl code.

Comment thread src/nix-build/nix-build.cc Outdated
if (runEnv && argc > 1 && !std::regex_search(argv[1], std::regex("nix-shell"))) {
script = argv[1];
if (access(script.c_str(), F_OK) == 0 && access(script.c_str(), X_OK) == 0) {
auto SCRIPT = std::ifstream(script);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No capitalized identifiers please.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also I would use tokenizeString(readFile(script), "\n") to read the file in lines.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@edolstra I only read one line here though.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah never mind, sorry

@shlevy
Copy link
Copy Markdown
Member Author

shlevy commented Aug 31, 2016

@edolstra for many of your comments: I figured since nix build is in the pipeline, it would be easiest to just dumbly translate the perl because this doesn't have to be maintained for too long (I hope!). Please confirm that you still would rather I remove the child process calls, argument handling, etc.

@edolstra
Copy link
Copy Markdown
Member

Yeah that's true. Let's leave it this way for now, we can integrate it will "nix build" later.

@shlevy
Copy link
Copy Markdown
Member Author

shlevy commented Aug 31, 2016

OK, which of these comments if any should I still address?

@edolstra
Copy link
Copy Markdown
Member

All except the one about using nix-instantiate please.

@shlevy
Copy link
Copy Markdown
Member Author

shlevy commented Aug 31, 2016

@edolstra Pushed changes

@edolstra
Copy link
Copy Markdown
Member

Thanks! Github is showing some merge conflicts though.

@shlevy
Copy link
Copy Markdown
Member Author

shlevy commented Aug 31, 2016

Yep, will merge and resolve once it LGTY.

@edolstra
Copy link
Copy Markdown
Member

Yes, looks good to me.

@shlevy shlevy merged commit 821380c into NixOS:master Aug 31, 2016
@vcunat
Copy link
Copy Markdown
Member

vcunat commented Aug 31, 2016

🎆

Ma27 pushed a commit to Ma27/nix that referenced this pull request Mar 8, 2026
We start this section with shortcomings of unsandboxed builds.

Fixes NixOS#1018.

Co-authored-by: eldritch horrors <pennae@lix.systems>
Change-Id: Ieb17e4340beab0c1197951813ae602de453a3fd9
Signed-off-by: Raito Bezarius <raito@lix.systems>
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.

4 participants