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

src: allow absolute paths for --env-file #49232

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions common.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,7 @@
'cflags_cc': [ '-fno-rtti', '-fno-exceptions', '-std=gnu++17' ],
'defines': [ '__STDC_FORMAT_MACROS' ],
'ldflags': [ '-rdynamic' ],
'ldlibs': [ '-lstdc++fs' ],
'target_conditions': [
# The 1990s toolchain on SmartOS can't handle thin archives.
['_type=="static_library" and OS=="solaris"', {
Expand Down
9 changes: 7 additions & 2 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@
#include <cstdio>
#include <cstdlib>
#include <cstring>
#include <filesystem>
Copy link
Member

Choose a reason for hiding this comment

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

I would recommend adding a comment here that <filesystem> is imported only to support path-processing operations and not actually performing any file system operations (which should always go through libuv)


#include <string>
#include <tuple>
Expand Down Expand Up @@ -858,8 +859,12 @@ static ExitCode InitializeNodeWithArgsInternal(
auto cwd = Environment::GetCwd(Environment::GetExecPath(*argv));

for (const auto& file_path : file_paths) {
std::string path = cwd + kPathSeparator + file_path;
per_process::dotenv_file.ParsePath(path);
if (file_path.is_absolute()) {
per_process::dotenv_file.ParsePath(file_path.string());
} else {
std::string path = cwd + kPathSeparator + file_path.string();
per_process::dotenv_file.ParsePath(path);
}
Comment on lines -861 to +867
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment explaining why it is necessary to construct an absolute path here?

Copy link
Member

Choose a reason for hiding this comment

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

I don't see why any of this is necessary, so I've opened #51425 as an alternative.

}

per_process::dotenv_file.AssignNodeOptionsIfAvailable(&node_options);
Expand Down
8 changes: 4 additions & 4 deletions src/node_dotenv.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,28 +8,28 @@ namespace node {
using v8::NewStringType;
using v8::String;

std::vector<std::string> Dotenv::GetPathFromArgs(
std::vector<std::filesystem::path> Dotenv::GetPathFromArgs(
const std::vector<std::string>& args) {
const auto find_match = [](const std::string& arg) {
const std::string_view flag = "--env-file";
return strncmp(arg.c_str(), flag.data(), flag.size()) == 0;
};
std::vector<std::string> paths;
std::vector<std::filesystem::path> paths;
auto path = std::find_if(args.begin(), args.end(), find_match);

while (path != args.end()) {
auto equal_char = path->find('=');

if (equal_char != std::string::npos) {
paths.push_back(path->substr(equal_char + 1));
paths.push_back(std::filesystem::path(path->substr(equal_char + 1)));
} else {
auto next_path = std::next(path);

if (next_path == args.end()) {
return paths;
}

paths.push_back(*next_path);
paths.push_back(std::filesystem::path(*next_path));
}

path = std::find_if(++path, args.end(), find_match);
Expand Down
3 changes: 2 additions & 1 deletion src/node_dotenv.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

#include "util-inl.h"

#include <filesystem>
#include <map>

namespace node {
Expand All @@ -22,7 +23,7 @@ class Dotenv {
void AssignNodeOptionsIfAvailable(std::string* node_options);
void SetEnvironment(Environment* env);

static std::vector<std::string> GetPathFromArgs(
static std::vector<std::filesystem::path> GetPathFromArgs(
const std::vector<std::string>& args);

private:
Expand Down
22 changes: 19 additions & 3 deletions test/parallel/test-dotenv-edge-cases.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@

const common = require('../common');
const assert = require('node:assert');
const path = require('node:path');
anonrig marked this conversation as resolved.
Show resolved Hide resolved
const fixtures = require('../common/fixtures');
const { describe, it } = require('node:test');

const validEnvFilePath = '../fixtures/dotenv/valid.env';
const nodeOptionsEnvFilePath = '../fixtures/dotenv/node-options.env';
const validEnvFilePath = path.relative(__dirname, fixtures.path('dotenv/valid.env'));
const absolutePath = fixtures.path('dotenv/node-options.env');
const relativePath = path.relative(__dirname, absolutePath);

describe('.env supports edge cases', () => {

Expand All @@ -18,7 +21,7 @@ describe('.env supports edge cases', () => {
`.trim();
const child = await common.spawnPromisified(
process.execPath,
[ `--env-file=${nodeOptionsEnvFilePath}`, `--env-file=${validEnvFilePath}`, '--eval', code ],
[ `--env-file=${relativePath}`, `--env-file=${validEnvFilePath}`, '--eval', code ],
{ cwd: __dirname },
);
assert.strictEqual(child.stderr, '');
Expand Down Expand Up @@ -51,4 +54,17 @@ describe('.env supports edge cases', () => {
assert.strictEqual(child.stderr, '');
assert.strictEqual(child.code, 0);
});

it('should support absolute paths', async () => {
const code = `
require('assert').strictEqual(process.env.CUSTOM_VARIABLE, 'hello-world');
`.trim();
const child = await common.spawnPromisified(
process.execPath,
[ '--env-file', absolutePath, '--eval', code ],
{ cwd: __dirname },
);
assert.strictEqual(child.stderr, '');
assert.strictEqual(child.code, 0);
});
});