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

feat(compile): support --env #24166

Merged
merged 25 commits into from
Jul 9, 2024
Merged

Conversation

HasanAlrimawi
Copy link
Contributor

@HasanAlrimawi HasanAlrimawi commented Jun 10, 2024

Supported the use of --env flag with the compile subcommand, so that the generated executable/binary file can access the passed env file.

The commit is made to address issue #22105 .

Use-case
deno compile --env=some_env_file file_using_env_vars.js -> executable/binary file "file_using_env_vars" is created.
file_using_env_vars to run it.

The problem that prevented the generated executable from finding the env file's environment variables

  • Through the compile subcommand flow, CliOptions are made by CliOptions::new and so is the env file passed.
  • Within the same function, environment variables found in the env file were loaded using the dotenvy crate which uses the std::env::set_var.
  • The std::env::set_var sets the env variables exclusively for the currently running process, and thus the env variables loaded within compile time are exclusive for the operation itself "The compile operation".
  • When you try to run the generated executable file, then a new process will take the responsibility, and this process doesn't have the environment variables loaded.

Solution of the problem
Pass the environment file to the executable/binary file, then load the environment variables whenever the file is ran.

Code changes

A) Added an attribute to hold the environment file name to Metadata struct within deno/cli/standalone/binary.rs, since the metadata is passed to the binary file by fn write_standalone_binary().

B) Added a getter for the environment file name to the CliOptions methods, since the Flags attribute that holds env_file attribute is private.

C) Added fn load_env_variables_from_env_file() to deno/cli/mainrt.rs which will be responsible for loading the env variables from the environment file at execution time.

@dsherret
@littledivy

@HasanAlrimawi
Copy link
Contributor Author

Hello @dsherret,
Just a kind reminder of this pull request.
Could you please review the latest changes? As this will help us proceed with applying more changes if needed or merging the changes if everything looks good.

@dsherret dsherret added this to the 1.45 milestone Jul 2, 2024
@dsherret dsherret changed the title fix: Added support for --env for the compile subcommand feat(compile): support --env Jul 2, 2024
cli/mainrt.rs Outdated
@@ -79,6 +80,8 @@ fn main() {
match standalone {
Ok(Some(future)) => {
let (metadata, eszip) = future.await?;
util::logger::init(metadata.log_level);
load_env_variables_from_env_file(metadata.env_file.as_ref());
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, just looking at this PR in detail. I'm not so sure about just providing a reference to the file because the file won't necessarily exist at runtime. Probably we need to embed the env variables into the executable and warn that we're doing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense.
I'll sort it out according to your vision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @dsherret ,

I made changes to embed the env variables from the env file into the generated executable and to warn the user about this action.
When running the executable, it will load the embedded variables as env variables.

@birkskyum
Copy link
Contributor

Maybe it's worth using --env-file like node (and bun). It's more self-explanatory.

@dsherret
Copy link
Member

dsherret commented Jul 9, 2024

@birkskyum the flag already shipped in Deno a while ago as --env. It just hasn't been part of deno compile.

Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @HasanAlrimawi!

@dsherret dsherret enabled auto-merge (squash) July 9, 2024 21:06
@dsherret dsherret merged commit fb63485 into denoland:main Jul 9, 2024
17 checks passed
@bartlomieju
Copy link
Member

@birkskyum I would suggest opening a separate issue if you think the flag name should be changed. It's still possible for Deno 2.

@birkskyum
Copy link
Contributor

@bartlomieju sure, I made a ticket here:

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