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

Use the stored activate binary appropriate for target platform #14

Merged
merged 9 commits into from
Dec 2, 2020

Conversation

antifuchs
Copy link
Contributor

@antifuchs antifuchs commented Nov 26, 2020

This addresses #13.

This PR rips out the current_exe field, preferring to write an activate-rs script that points to the platform-appropriate activation binary.

Then, the deploy binary attempts to use that activate-rs script to activate the profile closure.

I've successfully run a deploy to an amd64 linux installation from my intel macOS machine, so I suspect this might work (:

This should point to the correct path if the target platform differs
fro the deploying platform.
This gets rid of the "current_exe" vestige that has stuck around from
when this program was meant to be standalone; instead, we use
the (already known) path to the activate-rs wrapper, which
automatically uses the correct binary for the deploy target platform.
This gets rid of yet more code, so - win!
@antifuchs antifuchs changed the title Use stored activate binary, appropriate for the platform Use the stored activate binary appropriate for target platform Nov 26, 2020
@balsoft
Copy link
Member

balsoft commented Nov 26, 2020

There's a slight issue if someone tries to run a deploy-rs binary with this change on a flake that depends on a deploy-rs flake without this change. We probably need to handle this and when activate-rs is not found, throw an error that tells the user to run the same version as is pinned in the deployed flake.

@balsoft
Copy link
Member

balsoft commented Nov 26, 2020

This patch is fail-early and should make error messages a bit more clear:

diff --git a/src/utils/push.rs b/src/utils/push.rs
index 546ba3d..c6c28b9 100644
--- a/src/utils/push.rs
+++ b/src/utils/push.rs
@@ -4,6 +4,7 @@
 
 use std::process::Stdio;
 use tokio::process::Command;
+use std::path::Path;
 
 use thiserror::Error;
 
@@ -15,6 +16,12 @@ pub enum PushProfileError {
     BuildError(std::io::Error),
     #[error("Nix build command resulted in a bad exit code: {0:?}")]
     BuildExitError(Option<i32>),
+    #[error("Activation script deploy-rs-activate does not exist in profile.\n\
+             Did you forget to use deploy-rs#lib.<...>.activate.<...> on your profile path?")]
+    DeployRsActivateDoesntExist,
+    #[error("Activation script activate-rs does not exist in profile.\n\
+             Is there a mismatch in deploy-rs used in the flake you're deploying and deploy-rs command you're running?")]
+    ActivateRsDoesntExist,
     #[error("Failed to run Nix sign command: {0}")]
     SignError(std::io::Error),
     #[error("Nix sign command resulted in a bad exit code: {0:?}")]
@@ -91,6 +98,16 @@ pub async fn push_profile(
         a => return Err(PushProfileError::BuildExitError(a)),
     };
 
+    if ! Path::new(format!("{}/deploy-rs-activate", deploy_data.profile.profile_settings.path).as_str()).exists() {
+        return Err(PushProfileError::DeployRsActivateDoesntExist);
+    }
+
+    if ! Path::new(format!("{}/activate-rs", deploy_data.profile.profile_settings.path).as_str()).exists() {
+        return Err(PushProfileError::ActivateRsDoesntExist);
+    }
+
+
+
     if let Ok(local_key) = std::env::var("LOCAL_KEY") {
         info!(
             "Signing key present! Signing profile `{}` for node `{}`",

Although I guess it can be submitted in a separate PR.

@antifuchs
Copy link
Contributor Author

Making it fail early is a great solution; you have push permissions to my branch, but I can commit it also (:

@balsoft
Copy link
Member

balsoft commented Nov 26, 2020

Ah, ok, forgot about this feature of github :) I'll push this.

@antifuchs
Copy link
Contributor Author

One more thing that might make this less of a problem might be to find&document a good way of running the deploy via an apps entry in flake.nix itself: that way, the thing being run would be much more likely to be of the version that is pinned. I'll try to find a good one & submit a change to the readme next.

@balsoft
Copy link
Member

balsoft commented Nov 26, 2020

We're using either defaultApp = deploy-rs.defaultApp or apps = builtins.mapAttrs (_: deploy: { inherit deploy; }) deploy-rs.defaultApp in our repos.

@antifuchs
Copy link
Contributor Author

Oooh, the latter sounds really good - I like that it all turns into nix run ./#deploy when you set it up like that.

@kirelagin
Copy link

kirelagin commented Nov 26, 2020

(annoying prescriptivist hated by everyone joins the chat)

We are trying to keep this repo REUSE compliant and track copyright information correctly, so, @antifuchs, would you mind adding an SPDX-FileCopyrightText line for yourself in the header of the files that you made non-trivial changes to?

We should probably add this to our contributing guidelines.

@antifuchs
Copy link
Contributor Author

Hah, that's totally legit, @kirelagin. I'll check with my employer's legal team (who are currently enjoying a long weekend) to ensure I'm doing it right - will update this PR on Monday.

@kirelagin
Copy link

I'll check with my employer's legal team

Sounds great, thanks for understanding!

Just to be clear, we do not require a CLA, assigning the copyright to us, or anything like that. We would just like to explicitly record for posterity that it is you who are the author (and thus the copyright holder) of what you authored :).

However, all this applies only if you are writing this code on your own time. If you are doing this as part of your job, then you would need to get a permission from your employer to release your works under the license that we use (MPL-2.0), and the copyright header should contain not your, but your employer’s name.

Copy link
Contributor

@notgne2 notgne2 left a comment

Choose a reason for hiding this comment

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

This is all really good stuff, but there is a lot more code we can get rid of thanks to this, such as the NotNixStored error

src/utils/push.rs Show resolved Hide resolved
Now that we don't copy `activate` from the same directory as `current_exe`,
we can skip the check that the `current_exe` is reachable and in nix store.
Seems to have been removed by mistake in the previous cleanup
@antifuchs
Copy link
Contributor Author

antifuchs commented Dec 2, 2020

Checked with my legal friends at work & all is well, as expected - I added the copyright header & we're good to go.

@notgne2 notgne2 merged commit 05d21e0 into serokell:master Dec 2, 2020
@antifuchs antifuchs deleted the use-stored-activate branch December 2, 2020 23:38
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.

None yet

4 participants