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

inconsistent ways of expressing a file path #84

Closed
tagarwal opened this issue Jan 9, 2018 · 13 comments
Closed

inconsistent ways of expressing a file path #84

tagarwal opened this issue Jan 9, 2018 · 13 comments
Milestone

Comments

@tagarwal
Copy link

tagarwal commented Jan 9, 2018

I want the config files to be available for npm during runtime
Here is my BUILD.bazel file

package(default_visibility = ["//visibility:public"])

load("@build_bazel_rules_nodejs//:defs.bzl", "nodejs_binary")

nodejs_binary(
    name = "client-gen-service1",
    # Ordinarily this defaults to //:node_modules
    node_modules = "@foo//:node_modules",
    data = [":src/server.js",
            ":config/default.json",":config/dev.json"],
    entry_point = "__main__/client-gen/client-gen-service/src/server.js"
)

But when I run
bazel run :client-gen-service1 getting the following error

INFO: Running command line: bazel-bin/client-gen/client-gen-service/client-gen-service1
WARNING: NODE_ENV value of 'dev' did not match any deployment config file names.
WARNING: See https://github.com/lorenwest/node-config/wiki/Strict-Mode
WARNING: No configurations found in configuration directory:/private/var/tmp/_bazel_tagarwal/ea14abbe46cd7e70648aa164711eaeb5/execroot/__main__/bazel-out/darwin-fastbuild/bin/client-gen/client-gen-service/client-gen-service1.runfiles/__main__/config
WARNING: To disable this warning set SUPPRESS_NO_CONFIG_WARNING in the environment.
@tagarwal
Copy link
Author

tagarwal commented Jan 9, 2018

if I use filegroups to include the config files like

filegroup(
    name = "config_files",
    srcs = [
        "config/dev.json"
    ]
)

nodejs_binary(
    name = "client-gen-service1",
    # Ordinarily this defaults to //:node_modules
    node_modules = "@foo//:node_modules",
    data = [":src/server.js",
            ":config_files"],
    entry_point = "__main__/client-gen/client-gen-service/src/server.js",
)

Then I also I get this error
NFO: Running command line: bazel-bin/client-gen/client-gen-service/client-gen-service1
WARNING: NODE_ENV value of 'dev' did not match any deployment config file names.
WARNING: See https://github.com/lorenwest/node-config/wiki/Strict-Mode
WARNING: No configurations found in configuration directory:/private/var/tmp/_bazel_tagarwal/ea14abbe46cd7e70648aa164711eaeb5/execroot/main/bazel-out/darwin-fastbuild/bin/client-gen/client-gen-service/client-gen-service1.runfiles/main/config
WARNING: To disable this warning set SUPPRESS_NO_CONFIG_WARNING in the environment.

I can see the config files are available at

ls /private/var/tmp/_bazel_tagarwal/ea14abbe46cd7e70648aa164711eaeb5/execroot/__main__/bazel-out/darwin-fastbuild/bin/client-gen/client-gen-service/client-gen-service1.runfiles/__main__/client-gen/client-gen-service/config/
dev.json

@alexeagle
Copy link
Collaborator

if you want to load a file at runtime, you need to figure out the correct path to reference it, which should generally be [workspace name]/full/path/to/file
In your original post, you don't say what path you are trying to load from.

@alexeagle
Copy link
Collaborator

I'm not sure how to help more, please ask a specific question if you still need help.

@acehko
Copy link

acehko commented Feb 7, 2018

I have here a repository that test various ways of loading files in a nodejs_binary with javascript, nodejs_binary with ts_library and nodejs_image. It seems that there is no method which works in all cases.

For a nodejs_binary with javascript, the files can be loaded in two ways:

fs.readFile('full/path/to/file.txt', ...)
fs.readFile(__dirname + '../../relative/path/to/file.txt', ...)

For a nodejs_binary that depends on a ts_library there is only one way to open a file:

fs.readFile('full/path/to/file.txt', ...)

For a nodejs_image in both cases the result is the same:

fs.readFile(__dirname + '../../relative/path/to/file.txt', ...)

@alexeagle
Copy link
Collaborator

@acehko are you saying that you are able to read the file everywhere, but it's just confusing that it's not always the same path? Or is there a scenario that's broken?

@alexeagle alexeagle reopened this Feb 7, 2018
@acehko
Copy link

acehko commented Feb 7, 2018

Yes, I can read a file in all cases, but the path is different in each case.
I would expect there to be one way which works in all cases.

For example, I was expecting the example above ([workspace name]/full/path/to/file) to work, just like with typescript imports. But it doesn't.

I was using the full/path/to/file.txt way which works fine. When I tried to replace nodejs_binary with nodejs_image, the application is broken. But maybe this is an issue for rules_docker?

@alexeagle
Copy link
Collaborator

Yeah I'll catch up with @mattmoor about it, he probably has an opinion about whether to expose execroot paths or runfiles paths to users. I've been opting for runfiles paths everywhere so far.

@alexeagle alexeagle changed the title config files available during runtime for npm inconsistent ways of expressing a file path May 10, 2018
@alexeagle
Copy link
Collaborator

alexeagle commented May 12, 2018

@meisterT I could use some help to think through correct path handling in various places.

@alexeagle
Copy link
Collaborator

Some exploration into this:

  • Bazel launches toolchain processes with a CWD of execroot/wksp
  • Bazel's $(rootpath) helper for ctx.expand_location produces paths relative to runfiles/wksp

this suggests that Bazel wants to receive workspace-relative paths.
If we make this change, that means users are exposed to the external path segment, like here:
https://github.com/bazelbuild/rules_nodejs/blob/3a35786d8e78627f9e0783fbf45e1c960eaccdf8/internal/npm_package/packager.js#L122
becomes
require.resolve('external/nodejs/run_npm.sh.template')
I think based on bazelbuild/bazel#1262 maybe this is not intentional in Bazel, and once users start including external in their code, there is no going back.

Another complication is ESModule paths. I'm pretty sure that for ESModule imports, import {} from 'wksp/path/to/thing' is right because it matches deep commonjs imports in a package called wksp. But that makes things inconsistent too - in code you'll import with a workspace absolute path and in BUILD files you'd use workspace relative paths.

@dslomov too - in order to make the nodejs rules "Beta" I want to commit to what paths users should handle. Help!

alexeagle added a commit to alexeagle/rules_nodejs that referenced this issue May 14, 2018
BREAKING CHANGE:
This forces us to use workspace-relative paths since the rootpath helper returns this format.
Users are now exposed to the external path segment to reference other workspaces.
See discussion: bazel-contrib#84

Fixes bazel-contrib#32
@dslomov
Copy link
Contributor

dslomov commented May 16, 2018

Our longer-term plans in this space are here: https://docs.google.com/document/d/1qPOUeoqDA3eWFFXS1shWX1FT3e4BQB8yuSMrfQL4QrA/

The general idea is that you should make require.resolve('run_npm.sh.template') work independently of whether it is in main or external repository. For that, external/nodejs should be added to set of roots that require.resolve searches in for its files. Is this feasible?

For analogy, consider C++

  #include "foo/bar.h"

works independently of whether foo/bar.h is in main or in external repo. and this is achieved by passing appropriate -I flags to C++ compiler.

Does this make sense?

@alexeagle alexeagle added this to the Beta milestone Nov 30, 2018
@bryanjlee bryanjlee modified the milestones: Beta, 1.0 Aug 23, 2019
@bryanjlee
Copy link

We now use labels, so you don't have to figure out what string to put.

@gregmagolan
Copy link
Collaborator

I've read over this again and the string/label entry_point was not the issue but this has been inactive for a while so will leave it closed. Please file a new issue if it is still valid.

@mattsoulanille
Copy link
Contributor

Posting here since it's the first result for "bazel typescript open file path"

I have a node server that is built with nodejs_binary, rollup_bundle, and has several ts_library declarations. I was trying to open a png with fs.readFileSync for some server-side processing.

To make the file available, I added it to the data field of my nodejs_binary (path and filegorup work). Then, in typescript, I used require.resolve("my_workspace_name/.../the_file.png") to find the runtime path to the file. No rollup plugins were used.

A downside to this method is that any typescript library that depends on local data has to export a filegroup in addition to a ts_library. Listing the files in the data field of a ts_library seems to have not effect when built with nodejs_binary. I do not know enough about Bazel to know if this is intended behavior.

mattsoulanille added a commit to mattsoulanille/NovaJS that referenced this issue Jan 17, 2020
Local files can only be read if they're listed in the `data` field in `nodejs_binary`. The easiest way to get their paths is to use `require.resolve` on their path in the workspace, that is, `require.resolve("the_workspace_name/.../file.png"). See bazel-contrib/rules_nodejs#84.
alexeagle pushed a commit to alexeagle/rules_nodejs that referenced this issue Oct 17, 2020
Closes bazel-contrib#84

PiperOrigin-RevId: 177208267
alexeagle pushed a commit to alexeagle/rules_nodejs that referenced this issue Oct 18, 2020
Closes bazel-contrib#84

PiperOrigin-RevId: 177208267
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

No branches or pull requests

7 participants