Evertz/bzlgen

First impressions

Closed this issue · 7 comments

Toxicable commented

Hey guys, im pretty excited to use this project, it looks like it could suite out needs pretty well.
I just went through the process of setting it up and these are my notes I took as I was doing it:

  • --build_file_name should search for BUILD.bazel by default
  • Should be deployed to npm with direct deps and no BUILD files
  • --base_dir should default to cwd
  • should be able to use local buildozer installed from @bazel/buildozer

Once I got it setup I tried running npx bazel run @blzgen//src:bin -- ts libs/SOMELIB --base_dir=$(pwd) --build_file_name=BUILD.bazel
Which produced a commands.txt file:

new ts_library |//libs/SOMELIB/:__pkg__
add srcs some-sourcefile.ts index.ts another-sourcefile.ts|//libs/mission-mage/:
add deps //libs:SOMELIB //libs/SOMELIB/src:index //libs/SOMELIB/src/db:commenter @npm//@soa/common/core:core|//libs/SOMELIB/:

but then it throws:

ℹ  info      Generating BUILD.bazel file with type 'ts' for 'libs/SOMELIB/'
Too few arguments for command 'new', expected at least 2.

I'm not familiar with buildozer or it's commands but looks like the error is coming from there?

mattem commented

Hi @Toxicable, thank you for the notes and feedback, appreciate it.

--build_file_name should search for BUILD.bazel by default

Is BUILD.bazel the default for bazel now, or 'more standard'? Internally all our BUILD files are just 'BUILD'. Happy to switch just want to make sure it's the sensible default.

Should be deployed to npm with direct deps and no BUILD files

Yes, 100% working on that - hitting some internal complications. For the moment the releases can be installed from the tar on the releases page on GitHub. There is #4 for tracking this, hope to have it resolved soon.

--base_dir should default to cwd

This should already be the case, although I've not tried running it with npx. When installed via npm i -g it should use cwd, but when run with bazel run //src:bin it should use the BUILD_WORKSPACE_DIRECTORY env var as the working directory. Having said that, the bazel run command seems like it was done from another bazel workspace (as with the inclusion of @bzlgen)? I've not attempted that yet.

should be able to use local buildozer installed from @bazel/buildozer

Yes definitely.

It looks like you are missing the --load-mappings, there is a small section in the README about them. Thinking about it however, we should include the supported ones as default.
As some background as to why it's like that, internally we don't load directly from @npm_bazel_typescript for example, so never included them as default - another internal thing that spilled out that shouldn't have! - I'll put together a PR to address this.

These are the ones you'll likely need:

# load mappings
--load_mapping=sass_library=@io_bazel_rules_sass//sass:sass.bzl
--load_mapping=sass_binary=@io_bazel_rules_sass//sass:sass.bzl
--load_mapping=ts_library=@npm_bazel_typescript//:defs.bzl
--load_mapping=ng_module=@npm_angular_bazel//:index.bzl

... on a closer look...
It's missed the name for the new ts_library that it is attempting to generate. This should be derived from libs/SOMELIB, and result in SOMELIB. If you have a small reproduction of the issue or the codebase is opensource then I'll take a look.

Please let us know how you get on!

Toxicable commented

Is BUILD.bazel the default for bazel now, or 'more standard'? Internally all our BUILD files are just 'BUILD'. Happy to switch just want to make sure it's the sensible default.

Im not certain if it's the "more standard", but it is defiantely supported by bazel, I'd suggest an array of file names just incase `["BUILD", "BUILD.bazel"]

Yes, 100% working on that

Will folllow that ticket, thanks!

I just tried this out on the rules_nodejs codebase, except this time installing it with yarn yarn global add url @bazel/buildozer and it appears to be generating the build files correctly now.
I'll take another shot at using it this way on our codebase at work on monday and see how we get on.
The next issue I see though is how it's going to work with testing targets

mattem commented

Good to hear you had some success when installing with yarn. I'll see if I can track down what went wrong.

The next issue I see though is how it's going to work with testing targets

This functionality exists in our internal version, I've been looking at migrating it out here. There is some discussion in #9 for the generation of tests.

Toxicable commented

So I jsut tried it again on our codebase at work and ran into a few issues.

I made this little script to use bzlgen on our whole repository

const fs = require('fs');
const lodash = require('lodash')
const path = require('path');
const child_process = require('child_process');

function dir(parentPath) {
  const childPaths = fs.readdirSync(parentPath);

  const allPaths = childPaths.map(p => {
    p = path.join(parentPath, p);
    if(fs.statSync(p).isDirectory()) {
      return dir(p)
    } else {
      return p;
    }
  })
  return lodash.flatten(allPaths);
}

// const apps = dir('apps')
const apps = []
const libs = dir('libs')
const allPaths = [...apps, ...libs];

const dirs = new Set();
for(const p of allPaths.filter(p => p.endsWith('.ts'))) {
  dirs.add(path.dirname(p));
}

for(const d of Array.from(dirs.values()).sort()) {
  child_process.spawnSync(`npx`, `bzlgen ts ${d} `.split(' '), {stdio: 'inherit'})
}

with a .bzlgenrc

# bzl mappings
--load_mapping=sass_library=@io_bazel_rules_sass//sass:sass.bzl
--load_mapping=sass_binary=@io_bazel_rules_sass//sass:sass.bzl
--load_mapping=ts_library=//libs/bazel-rules:defaults.bzl
--load_mapping=ng_module=@npm_angular_bazel//:index.bzl
# ts mappings
--label_mapping=rxjs/operators=@npm//rxjs
--label_mapping=@workspace/appname=//libs/appname
# other opts
--buildozer_binary=node_modules/.bin/buildozer
--build_file_name=BUILD.bazel

Our first issue was likely caused from our use of https://www.npmjs.com/package/barrelsby
Which casuses bzlgen to create invalid deps.
These are the generated build files
libs/browser/src/some/path/icon/svg

load("//libs/bazel-rules:defaults.bzl", "ts_library")

ts_library(
    name = "svg",
    srcs = [ ... ],
)

libs/browser/src/some/path/icon

load("//libs/bazel-rules:defaults.bzl", "ts_library")
ts_library(
    name = "icon",
    srcs = [ ... ],
    deps = [
        ":svg",
        "//libs/browser/common/src/components/core/icon/svg:index",
        "@npm//@angular/core",
        "@npm//@angular/material",
        "@npm//@angular/platform-browser",
    ],
)

I think the reason for this is that every dir with .ts files under libs has a index.ts file which exports all files and dirs, so that we can use imports like @workspace/libname with the TS path

    "paths": {
      "@workspace/*": ["libs/*"]
    },

For example libs/browser/src/some/path/icon/index.ts looks like

export * from './svg/index';
export * from './file'; ...
Toxicable commented

I pulled down the repository and added a unit test, however I wasn't able to fix it.

The issue is when you import from a file in another relative package, bzlgen will use the file name as the target, however if you used bzlgen to make that target, it'll be named after the dir, not the file.
Meaning that the target produced from importing relative libs should be the dir name not the filename

Here's my test:

    mockfs({
      '/home/workspace/src/some': {
        'one.ts': `import {SOME_CONST} from './nested/two'; `
      },
      '/home/workspace/src/some/nested': {
        'two.ts': `export const SOME_CONST = '';`
      }
    });

which produces

add deps //src/some/nested:two|//src/some:some

But i'd expect to get:

add deps //src/some/nested:nested|//src/some:some

I've also sent though another PR which will fix deep imports for any package.
https://github.com/Evertz/bzlgen/pull/19/files

mattem commented

Ah yes, this is likely an artifact of how our packages are made up and to an extent that it's been primarily used for Angular where we'd expect one component and module per folder, and the naming convention to be pretty tight. I'll spin this off to another issue to track it.

Toxicable commented

Yeah I don't think this issue has really a done condition, so ill close it now and make some smaller issues