Skip to content

Add option to build on the target host #175

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

Merged
merged 1 commit into from
Nov 18, 2022
Merged

Conversation

PhilTaken
Copy link
Collaborator

This PR adds the option to build the derivation on the target server instead of locally.
The derivation will first be uploaded to the target system and then built in the remote store.
Any required external dependencies will be fetched to the target host using the target system's substituters.

Fixes #12

for extra_arg in extra_build_args {
c.arg(extra_arg);
}
c.args(extra_build_args);
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 also add --remote-build flag to the command line arguments of deploy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

as a "build all deployments on the respective target servers" override?
the current implementation only builds selected builds remotely, but that could be a good idea

Copy link
Member

Choose a reason for hiding this comment

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

as a "build all deployments on the respective target servers" override?

Yes, I imagine that it might be useful when one doesn't have decent internet connection or has limited traffic

@PhilTaken PhilTaken force-pushed the philtaken/remote-building branch 2 times, most recently from 02b3990 to f136b54 Compare November 15, 2022 11:37
for extra_arg in data.extra_build_args {
build_command.arg(extra_arg);
}
build_command.args(data.extra_build_args);

let build_exit_status = build_command
// Logging should be in stderr, this just stops the store path from printing for no reason

Choose a reason for hiding this comment

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

When I try this out and run it on my machine, it fails on line 181

  if !Path::new(
        format!(
            "{}/deploy-rs-activate",
            data.deploy_data.profile.profile_settings.path
        )
        .as_str(),

because, as the system is remote and the closure was built remotely, this path is not going to exist in my store or on my system.

I'm not sure I have a great solution for that, but I wanted to point that out cause I'm trying this out in anticipation of managing remote machines via macOS

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps, in this case, activation should happen on the target as well 🤔

Copy link
Member

Choose a reason for hiding this comment

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

FTR, I have the same issue even with NixOS -> NixOS deployment 🙈

Choose a reason for hiding this comment

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

How I ended up "sorta solving this" myself was to take the path that deploy info spits out and then dead-reckoning run a little bash function with that path as the input.

function activate-on-host(){                                                            
  printf -v profile %q "$1"                                                          
  ssh user@FQDN "export PROFILE=$profile ; bash $profile/deploy-rs-activate" 
}                                                                                    

Which, indeed, simply runs the activation on the target

Copy link
Collaborator Author

@PhilTaken PhilTaken Nov 16, 2022

Choose a reason for hiding this comment

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

I see, I did not encounter that error, very likely because I had it built locally before I tested remote building

the push_profile function does quite a lote at this point and most of it assumes the derivation is being built locally

splitting it up into a function to handle building locally and another to handle remote builds should resolve some of those difficulties

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

besides, contrary to its name it also handles the building, not just the pushing 🤔

@PhilTaken PhilTaken force-pushed the philtaken/remote-building branch 3 times, most recently from f7f8212 to 20be00d Compare November 16, 2022 17:33
@PhilTaken
Copy link
Collaborator Author

@hazelweakly could you try again with the newest changes?

make sure to gc before 😉

Copy link
Member

@rvem rvem left a comment

Choose a reason for hiding this comment

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

Tried both remote and non-remote builds, worked for me 🎉

src/push.rs Outdated
let mut build_command = Command::new("nix");
build_command
.arg("build").arg(derivation_name)
.arg("--eval-store") .arg("auto")
Copy link
Member

Choose a reason for hiding this comment

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

Tiny nitpick:

Suggested change
.arg("--eval-store") .arg("auto")
.arg("--eval-store").arg("auto")

@hazelweakly
Copy link

@PhilTaken I can confirm that it works for me too (remotely). Thank you! This is very nice :)

@PhilTaken PhilTaken force-pushed the philtaken/remote-building branch from 20be00d to d0c8665 Compare November 18, 2022 12:42
@PhilTaken PhilTaken merged commit 2a3c5f7 into master Nov 18, 2022
@delete-merged-branch delete-merged-branch bot deleted the philtaken/remote-building branch November 18, 2022 18:46
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.

Build on target server?
3 participants