Add NodeJSSourcesInfo provider to allow for better layering in docker rules. #283

Globegitter · 2018-08-14T22:06:16Z

Right now the nodejs_image packs unnecessary weight, it seems about an extra 20-30MB in total, and about 60MB in the top layer which allows for quite poor incremental builds.

One thing was the @nodejs//:node_runfiles which includes yarn and npm and a few other things. I am not quite sure why that is necessary to be in the runfiles of each nodejs binary, so I created a minimal_node_runfiles, which excludes those extra files. The next point was, that it currently does not seem to be possible to get access to just the sources of the binary, which is needed to get better layering, so I included a new provider which rules_docker is going to make use of. I am not sure if that is the best way of doing it, but the only one I could get to work.

Incremental builds are really fast now and in the rules_docker example a change in the app now just has a 60KB top layer to be updated compared to 60+MB before. In one of our apps it is reducing the top/app layer size from ~223MB to ~28MB and reduces the layer rebuild time from ~9/10 to ~3s.

Also from what I could figure out, returning transitive_files, files and collect_data in DefaultInfo is redundant so I removed it and all still seems fine.

Globegitter · 2018-08-14T22:13:49Z

This is the PR in rules_docker: bazelbuild/rules_docker#487

Also this is complaining about exporting SourcesInfo being the wrong casing. How is the best way to export a provider to be used by another rule?

nimerritt · 2018-09-07T14:25:50Z

Nice work! We were experience slow builds when using the docker images as well! Hope this gets merged! Thanks 👍

Globegitter · 2018-10-02T09:47:06Z

@alexeagle any thoughts on this? This PR is blocking the merge/finalisation of the PR in rules_docker: bazelbuild/rules_docker#487

alexeagle · 2018-10-23T21:07:04Z

sorry it got overlooked, I'll TAL today

alexeagle

this PR is missing some test coverage which proves the new provider does the thing it's supposed to

alexeagle · 2018-10-23T22:32:07Z

internal/node/node_repositories.bzl

@@ -239,21 +239,10 @@ alias(name = "npm", actual = "{npm}")
 alias(name = "yarn", actual = "{yarn}")
 filegroup(
  name = "node_runfiles",


in your change, you introduced a new minimal_node_runfiles but then I believe the existing one became unreferenced.

It's possible that users downstream referenced @npm//:node_runfiles and rely on the old content, we should figure that out by vetting this PR on downstream repos.

alexeagle · 2018-10-23T22:32:43Z

internal/node/node.bzl

@@ -35,6 +35,11 @@ def _trim_package_node_modules(package_name):
        segments += [n]
    return "/".join(segments)

+NodeJSSourcesInfo = provider(
+    doc = """Minimal files needed to run a NodeJS program, without npm packages""",


I started a doc string here but it needs to answer more questions

how is this intended to be used?

where do the npm packages come from in the consuming end?

Good point about "where do the npm packages come from" - in the new way of having the fine-grained node modules we indeed need to provide these here in the provider also. I updated that.

Not sure what exactly you mean with " * how is this intended to be used", but I did add a sentence for it.

Globegitter · 2018-10-24T07:22:08Z

Thanks for the comments I will have some time to go over it tomorrow.

Toxicable · 2018-12-04T03:44:15Z

@Globegitter any updates here? is there anything I can do to help out?
We're in need to this fix to docker images

Globegitter · 2018-12-06T14:03:37Z

@Toxicable Sorry I just have not had any time since there was quite a delay since PR opening and the review. My hope is to get around to this before the end of the year.

tmc · 2019-02-02T21:53:35Z

@Globegitter it looks like this work needs to be rebased.

Globegitter · 2019-02-14T13:08:43Z

Just brought this up-to-date with latest master and responded to the comments. I will update bazelbuild/rules_docker#487 next but hopefully this is in a state now it can get merged, or at least very close to it.

Globegitter · 2019-02-15T09:18:55Z

I just updated bazelbuild/rules_docker#487 @Toxicable and @tmc so if anyone wants to test this it is ready :)

Toxicable · 2019-02-15T09:39:58Z

@Globegitter Awesome! thanks man, I'll give it a try tomorrow and see how much of a difference it makes

Toxicable · 2019-02-16T19:15:22Z

Just gave this a shot and this was the result:
Before:

...: Loading layer [=>]  10.24kB/10.24kB
...: Loading layer [=>]  10.24kB/10.24kB
...: Loading layer [=>]    162MB/162MB

After:

...: Loading layer [=>]  10.24kB/10.24kB
...: Loading layer [=>]  39.91MB/39.91MB
...: Loading layer [=>]  10.24kB/10.24kB
...: Loading layer [=>]  10.24kB/10.24kB
...: Loading layer [=>]  2.806MB/2.806MB

I cant actually run the container since it throws this error on mac:

/app/apps/appname/image.binary_bin.runfiles/nodejs/bin/node: line 19: /app/apps/appname/image.binary_bin.runfiles/nodejs/bin/nodejs/bin/node: cannot execute binary file: Exec format error

But I was getting that error beforehand so it's not related to this PR.

Im a little confused as to where the node_modules layer is though.
layer 2 looks like the node runtime, but there are 3 empty layers?
Also the overall size dropped from 162 -> 42 - where are all those other files?

alexeagle · 2019-02-19T06:04:05Z

internal/node/generate_build_file.js

-  // *.pyc files are generated during node-gyp compilation and include
-  // absolute paths, making them non-hermetic.
-  // See https://github.com/bazelbuild/rules_nodejs/issues/347
-  const excludedFiles = ['.md', '.html', '.pyc'];


where did this code go? Don't we still need to exclude these non-hermetic inputs?

This is just bringing the changes that you did here: 3380dd7#diff-67b0f7c2b38c3bbb6dd40c85eb923174L242 up-to-date with the new code. Basically node_runfiles includes a whole load of extra stuff like yarn and everything else that node ships with but imo that seems unnecessary, as if someone e.g. needs yarn they should just add it as a dependency explicitly rather than everyone having to pay the cost for this. And our own internal app is still working fine after this change and all tests here are still passing so it seems to me these files are not needed for a nodejs_binary and they add quite a bit of weight to the resulting docker image.

Globegitter · 2019-02-19T11:32:34Z

@Toxicable that might be because you are running on on a mac, but yes now there will be 5 layers (which in theory could be optimised into 3 layers I am just not sure how right now), 3 are layers which just have node specific stuff and should not change on most rebuilds.Yes ideally these would be merged into one layer but I think it is an acceptable first step for now. Then the fourth layer should have your node_modules. You can do docker run -it --entrypoint=bash bazel/image:target to ssh into the container and inspect the files directly and you should see the node modules when running ls -lah external/ in the docker container.

And part of the reason that the image is significantly smaller is that a lot of auxiliary data is gone now, like yarn was packaged into the image before as well. And maybe you where still passing in the whole of node_modules before?

Toxicable · 2019-02-20T02:32:06Z

@Globegitter Im inspecting the container but cant find any node_modules for this application.

root@cb0bd373a87a:/app/apps/appname/image.binary_bin.runfiles/workspacename/external# ls
bazel_tools  nodejs  workspacename

I can see we have about 10k files from node_modules so even if my manual investigation cant find them in the container we can see that none of the sizes I stated above 10.24kb, 39.91mb, 2.806mb could container that many files

For context this is our docker macro

def docker_image(name, entry_point, data, repository, ports = ["80"], **kwargs):
    container_name = name + ".container"

    nodejs_image(
        name = name,
        entry_point = entry_point,
        node_modules = "//:empty_node_modules",
        data = data,
        **kwargs
    )

    container_image(
        name = container_name,
        base =  name,
        ports = ports,
    )

    container_push(
        name = name + ".push",
        image = container_name,
        format = "Docker",
        registry = "...",
        repository = repository,
        tag = "{BUILD_SCM_VERSION}",
    )

and this is our target

docker_image(
  name = "image",
  entry_point = "soa/apps/appname/src/main.js",
  data = [":ts_server", ":repo_status_stamp"] + ["@npm//somemodule" ...] ,
  repository = "..."
)

After inspecting the dist folder I can see that the node_modules are included in the image.binary_bin.sh.runfiles_manifest file but they are definately not in our container

Looking at the files produced by
I can see

image-layer.manifest - references application code
image.0-layer.manifest - refernces node
image.1-layer.manifest - refernces node
image.2-layer.manifest - refernces node args
image.3-layer.manifest - referneces nothing
image.runfiles_manifest - references all the other image.x files
image.binary_bin.sh.runfiles_manifest - references node-modules

Globegitter · 2019-02-20T08:10:42Z

@Toxicable, ah I got the issue. You are still passing a node_modules into nodejs_image. Remove the attribute entirely and try again.

Toxicable · 2019-02-20T19:39:19Z

@Globegitter Ah! progress, ok now we're seeing more reasonable numbers:

...: Loading layer [=>]  107.9MB/107.9MB
...: Loading layer [=>]  10.24kB/10.24kB
...: Loading layer [=>]  39.15MB/39.15MB
...: Loading layer [=>]  10.24kB/10.24kB
...: Loading layer [=>]  50.07MB/50.07MB
...: Loading layer [=>]  2.826MB/2.826MB

Although it isn't quite working just yet.
When I run it I get the runtime error saying we're missing transitive npm deps

example:

ts_library(
    name = "library",
    deps = [
      ":lib",
    ] + ["@npm//libdep"],
    srcs = glob(["*.ts", "src/**/*.ts"], exclude = ["src/**/*.spec.ts"]),
)
ts_library(
    name = "application",
    deps = [
      ":library",
    ] + ["@npm//applicationdep"],
    srcs = glob(["*.ts", "src/**/*.ts"], exclude = ["src/**/*.spec.ts"]),
)

docker_image(
  name = "image",
  entry_point = "soa/apps/appname/src/main.js",
  data = [":application"] + NPM_DEPS,
  repository = "..."
)

When i boot the image I get the runtime error that it cant find the libdep, if I add the lib dep to the application set of npm deps then it works.

It worked before with this setup - having out npm deps stread out acrross the libs that actually use them
Also just to config; bashing into the container I can clearly see all the direct dependancies of the application under external/npm/node_modules but not the transitive ones.

Globegitter · 2019-02-21T07:50:40Z

Ah good catch, thanks for sharing that. Should be able to fix that.

tmc · 2019-02-21T15:40:20Z

Nit but thee seems to be a typo in pr title: SorucesInfo

gregmagolan · 2019-02-21T19:13:41Z

This is looking good. Thanks for updating @Globegitter. My only concern is the node runfiles on Windows. I'll do some testing today and have a closer look.
Is this at the point that you're ready to land it or are there some fixes that came up with @Toxicable's testing?

Globegitter · 2019-02-21T21:49:17Z

@gregmagolan, yep I still need to look into the new issue discovered. Should get to it in the next few days. And I'll see about adding a test for this also.

Globegitter · 2019-03-10T21:20:29Z

@Toxicable I just tried to reproduce and as far as I can see neither the native nodejs_binary nor rollup_bundle load transitive npm dependencies automatically. Does running :image.binary work for you even without this PR?

Toxicable · 2019-03-11T01:06:31Z

@Globegitter Just tried running :image.binary and it's definately loading up npm deps from transitive libs.
We run all out applications with nodejs_binary and they 100% have npm deps defined in our libs that aren't in our applications ts_libraray

Globegitter · 2019-03-11T08:16:19Z

@Toxicable Hmm strange, I added a repro repository here: https://github.com/Globegitter/ts_nodejs_binary_repro Is your behaviour different to that? Am I doing something wrong? Are we using different versions of rules_nodejs?

Either way I will also add a PR here to test that case as we do not have that.

Toxicable · 2019-03-11T09:29:22Z

@Globegitter Ok this is super weird.
It took me about an hour but if you add any lib from @npm into the nodejs_binary.data then it'll all of a sudden resolve transitive deps.

From your reproduction: add another module, such as date-fns and run it again.
We have a macro which wraps ts_library and adds @npm//source-map-support so that's why we've never seen this behaviour, since at least one npm module was added to the nodejs_binary always.

If this is a bug; I do hope we don't have to redeclare npm deps that a lib used in the future.

Example:

nodejs_binary(
  name = "app",
  entry_point = "__main__/app/main.js",
  data = [
    ":main",
    "@npm//date-fns",
    # Uncomment to fix
    # "@npm//five",
  ],
)

Globegitter · 2019-03-11T13:40:03Z

@Toxicable Ahh very interesting, that does sound like a bug to me indeed. I will open up a PR to add a test for both behaviours.

Globegitter · 2019-03-11T21:05:23Z

@Toxicable I added tests here: #612 funnily enough I am not getting transitive deps loading working at all there for the nodejs_binary. Well either way, now we have something that tests this and then I can look at fixing it and for it to stay fixed.

Toxicable · 2019-03-11T21:13:56Z

So you wern't able to reproduce it?
I forked your repo here and these are the only changes I made to reproduce it: https://github.com/Toxicable/ts_nodejs_binary_repro
Toxicable/ts_nodejs_binary_repro@99e7954

Globegitter · 2019-03-11T22:10:25Z

@Toxicable Sorry for not being clear, I do get it to work as you said in the commit you linked. But I have different behaviour here in the repo. Well either way I will investigate.

gregmagolan · 2019-04-09T02:59:32Z

Should be unblocked when #675 lands which replaces #612

gregmagolan · 2019-04-11T17:22:21Z

#675 has landed which adds transitive npm deps support via the NodeModuleSources provider

Toxicable · 2019-05-09T08:06:17Z

@Globegitter Hey man, any chance you got some ttime to polish this off now it's unblocked?

Globegitter · 2019-06-13T12:21:47Z

Replaced this now by #863

Globegitter mentioned this pull request Aug 14, 2018

Fix nodejs_image layering bazelbuild/rules_docker#487

Closed

Globegitter and others added 4 commits October 23, 2018 15:14

Add SorucesInfo provider to allow for better layering in docker rules.

a101eff

Export SourcesInfo.

fec940e

Update to exclude fin-grained deps in SourcesInfo provider.

d22bb3c

Cleanup

3380dd7

alexeagle force-pushed the mp-nodejs-image-layering branch from a6142cf to 3380dd7 Compare October 23, 2018 22:16

alexeagle reviewed Oct 23, 2018

View reviewed changes

globegitter added 2 commits February 14, 2019 09:27

Manually merge latest master.

6b35de4

Also provide node_module sources.

ae7ab76

Globegitter requested review from gregmagolan and kyliau as code owners February 14, 2019 08:39

globegitter added 4 commits February 14, 2019 09:41

Remove obsolete comment.

984a031

Lint and syntax fixes.

a87eadb

Depset fix.

a5121bc

Fix for node_runfiles.

0f6201a

alexeagle reviewed Feb 19, 2019

View reviewed changes

gregmagolan changed the title ~~Add SorucesInfo provider to allow for better layering in docker rules.~~ Add NodeJSSourcesInfo provider to allow for better layering in docker rules. Feb 21, 2019

Globegitter mentioned this pull request Mar 11, 2019

Add e2e tests/examples of transitive npm deps for ts_library - nodejs_binary interop #612

Closed

10 tasks

Globegitter mentioned this pull request May 28, 2019

Remove runfiles. #798

Merged

12 tasks

Globegitter mentioned this pull request Jun 13, 2019

Add NodeJSRuntimeInfo provider #863

Closed

12 tasks

Globegitter closed this Jun 13, 2019

Add NodeJSSourcesInfo provider to allow for better layering in docker rules. #283

Add NodeJSSourcesInfo provider to allow for better layering in docker rules. #283

Conversation

Globegitter commented Aug 14, 2018 • edited Loading

Globegitter commented Aug 14, 2018

nimerritt commented Sep 7, 2018

Globegitter commented Oct 2, 2018

alexeagle commented Oct 23, 2018

alexeagle left a comment

Choose a reason for hiding this comment

alexeagle Oct 23, 2018

Choose a reason for hiding this comment

alexeagle Oct 23, 2018

Choose a reason for hiding this comment

Globegitter Feb 14, 2019

Choose a reason for hiding this comment

Globegitter commented Oct 24, 2018

Toxicable commented Dec 4, 2018

Globegitter commented Dec 6, 2018

tmc commented Feb 2, 2019

Globegitter commented Feb 14, 2019

Globegitter commented Feb 15, 2019

Toxicable commented Feb 15, 2019

Toxicable commented Feb 16, 2019

alexeagle Feb 19, 2019

Choose a reason for hiding this comment

Globegitter Feb 19, 2019

Choose a reason for hiding this comment

Globegitter commented Feb 19, 2019 • edited Loading

Toxicable commented Feb 20, 2019 • edited Loading

Globegitter commented Feb 20, 2019

Toxicable commented Feb 20, 2019

Globegitter commented Feb 21, 2019

tmc commented Feb 21, 2019

gregmagolan commented Feb 21, 2019 • edited Loading

Globegitter commented Feb 21, 2019

Globegitter commented Mar 10, 2019 • edited Loading

Toxicable commented Mar 11, 2019

Globegitter commented Mar 11, 2019

Toxicable commented Mar 11, 2019

Globegitter commented Mar 11, 2019

Globegitter commented Mar 11, 2019

Toxicable commented Mar 11, 2019

Globegitter commented Mar 11, 2019

gregmagolan commented Apr 9, 2019

gregmagolan commented Apr 11, 2019

Toxicable commented May 9, 2019

Globegitter commented Jun 13, 2019

Globegitter commented Aug 14, 2018 •

edited

Loading

Globegitter commented Feb 19, 2019 •

edited

Loading

Toxicable commented Feb 20, 2019 •

edited

Loading

gregmagolan commented Feb 21, 2019 •

edited

Loading

Globegitter commented Mar 10, 2019 •

edited

Loading