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

build: add Bazel build system #54

Closed
wants to merge 10 commits into from
Closed

Conversation

CaerusKaru
Copy link

This is a continuation of #4, which got a little messed up when the repo was made public.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@CaerusKaru
Copy link
Author

CaerusKaru commented Mar 8, 2020

Apologies to anyone tracking for the commit spam. Here's the current status:

  • All build steps pass locally and on CI. This includes all packaging and release steps through jsii-release
  • All tests currently build and run correctly. The only errors are due to out-of-date snapshots. Functionality to update the snapshots through Bazel is pending
  • All examples build and generate the imports files through Bazel correctly, in addition to tests. However example synthesis is still pending (though is mostly done). Though strictly speaking they synthesize correctly, they don't end up in the correct directory for easy use

tl;dr only two things left: snapshot test updation through Bazel, example synthesis through Bazel

@CaerusKaru
Copy link
Author

Progress update:

  • Jest tests all pass and snapshot updation is covered and working. The CLI tests need an NPM patch so that require.resolve will work correctly with JSII
  • Example synthesis is still pending, but is the only major hurdle left

Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

Haven't finished reviewing everything, but here is an initial round of comments.

.github/.bazelrc Outdated Show resolved Hide resolved
Comment on lines 13 to 14
- run: yarn bazelrc:ci
- run: yarn bazelrc
Copy link
Contributor

Choose a reason for hiding this comment

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

What are these doing?

Copy link
Author

Choose a reason for hiding this comment

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

In order for Bazel to get setup correctly, we need to move the CI bazelrc file into place (/etc/bazel.bazelrc) and then we need to initialize Bazel with the CI's PATH variables (any any other ENV we need at build time). I can add comments to this file if you think it will help.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need two commands here?

Copy link
Author

Choose a reason for hiding this comment

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

The first one moves the top-level Bazel RC file into place, only on CI. The second is a command used by all consumers of the library. We could fold it into prebuild (I have a comment on that in the package.json right now), but both commands are required. We could rename them too to make it clearer.

BUILD.bazel Outdated Show resolved Hide resolved
BUILD.bazel Show resolved Hide resolved
WORKSPACE Show resolved Hide resolved
packages/BUILD.bazel Show resolved Hide resolved
packages/cdk8s-cli/BUILD.bazel Outdated Show resolved Hide resolved
packages/cdk8s-cli/BUILD.bazel Outdated Show resolved Hide resolved
packages/cdk8s-cli/jest.config.js Outdated Show resolved Hide resolved
packages/cdk8s-cli/snapshotResolver.js Outdated Show resolved Hide resolved
@CaerusKaru
Copy link
Author

CaerusKaru commented Mar 11, 2020

Progress update:

  • CLI tests are working fine now. Snapshot updation has a minor issue in the examples, but tests run regardless Snapshot updation is fully working in all cases.
  • Examples synth is still pending
  • Fixed a packaging issue that preventing yaml from getting bundled with the library. Also added the LICENSE file

@CaerusKaru
Copy link
Author

All systems go! There is one precondition for doing example synthesis that I'm trying to make easier, but in the meantime the base functionality for the entire repo has been replicated with Bazel in some form.

@eladb
Copy link
Contributor

eladb commented Mar 18, 2020

@CaerusKaru can you do one last merge from master and I'll pick this up

@CaerusKaru
Copy link
Author

@eladb Should be all set 👍

@eladb
Copy link
Contributor

eladb commented Mar 20, 2020

I am getting the following message when running "yarn install":

Unrecognized patch file in patches directory rules_nodejs.patch

experimental = True,
jsii_targets = JSII_TARGETS,
peer_deps = {
"constructs": CONSTRUCTS_PACKAGE_VERSION,
Copy link
Contributor

Choose a reason for hiding this comment

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

I expected constructs to also appear in devDeps here. I actually want to make sure the devDep is pinned on 2.0.0 because that's the lowest version we currently support.

Copy link
Author

Choose a reason for hiding this comment

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

devDeps are not needed for package publishing, otherwise package version is handled solely at the root package.json level.

})

package_json(
name = "package_json",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this name really needed? This produces a package.json file, can we have a default on the name?

Copy link
Author

Choose a reason for hiding this comment

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

We can have a macro that defaults the name, sure. But the name corresponds to the target and is a mandatory parameter for Bazel actions.


package_json(
name = "package_json",
package_name = MODULE_NAME,
Copy link
Contributor

Choose a reason for hiding this comment

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

seems inconsistent. In some places this is called module_name and here package_name. Can we normalize?

Copy link
Author

Choose a reason for hiding this comment

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

module_name is a ts_library parameter that we inherit (this allows us to use jsii_library as a dep for ts_library targets). package_name can be converted to module_name for consistency, but I thought package_name was clearer here.

exclude = ["test/**"],
),
module_name = MODULE_NAME,
package_json = ":package_json",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can probably be omitted if we have a default name for the package_json rule, no?

Copy link
Author

Choose a reason for hiding this comment

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

Only if we also adapt the jsii_library to be a macro that accepts the default. In my mind, we'd have one jsii macro that sets up the package_json, jsii_library and jsii_package targets all in one. This has the benefit of being DRY but also allowing the user to trigger the individual actions.

Comment on lines +52 to +55
"@npm//@types/json-stable-stringify",
"@npm//@types/node",
"@npm//@types/yaml",
"@npm//constructs",
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like a duplicate from package.json, can we remove?

Copy link
Author

Choose a reason for hiding this comment

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

Only if we incorporate the deps aspect into the package_json rule, but sure.

experimental = True,
jsii_targets = JSII_TARGETS,
peer_deps = {
"constructs": CONSTRUCTS_PACKAGE_VERSION,
Copy link
Contributor

Choose a reason for hiding this comment

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

why isn't this @npm//constructs or something like that?

Copy link
Author

Choose a reason for hiding this comment

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

This is translated raw to the package.json. It doesn't incorporate the actual package as a dep (though it could if we decide that's the way to go).

Comment on lines +38 to +39
"json-stable-stringify": "^1.0.1",
"yaml": "^1.7.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

I expected these to reference the @npm// scope somehow instead of hard code the versions here, no?

Copy link
Author

Choose a reason for hiding this comment

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

That could be incorporated into the package.json rule, sure.

],
)

jsii_package(
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference between jsii_library and jsii_package? Why do we need both?

Copy link
Author

Choose a reason for hiding this comment

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

jsii_library -> jsii
jsii_package -> jsii-pacmak

Each requires separate tooling, and so should remain separate (when you develop a project, you don't invoke jsii-pacmak every time)

},
})

package_json(
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if we even need package_json here. Seems like we can probably merge it (and jsii_package) into jsii_library which will just do everything. I don't see any reason to split those up really.

Basically, I'd expect this file to look something like this:

jsii_library(
    name = "lib",
    desc = "Cloud Development Kit for Kubernetes",
    stability = "experimental",
    jsii_targets = JSII_TARGETS,
    srcs = glob(
        [ "**/*.ts", "README.md" ],
        exclude = ["test/**"],
    ),
    module_name = "cdk8s",
    dev_deps = [
        "@npm//@types/json-stable-stringify",
        "@npm//@types/node",
        "@npm//@types/yaml",
        "@npm//constructs", # this needs to be pinned to 2.0.0
    ],
    peer_deps = [
        "@npm//constructs",  # this needs to be ^2.0.0 (not sure how to express)
    ],
    bundled_deps = [
        "json-stable-stringify",
        "yaml"
    ]
)

Something along those lines.

Copy link
Author

Choose a reason for hiding this comment

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

I think it's important to have separate, same with jsii_package, because it's a separate action. We can create a macro to encapsulate the behavior you're talking about, but leaving these compose-able gives us some advantages. E.g. we don't need to recompile a library every time we want to package it. Or we can reuse the package.json elsewhere without compiling the library (important for test path resolution).

@eladb
Copy link
Contributor

eladb commented Mar 20, 2020

@CaerusKaru this is not good enough yet... too much duplicated information all over the place. Repeating that one of the main motivations for me is to "DRY"-up all the info that we need to supply in order to build. I want to be able to provide the minimum amount of information and never repeat myself if not needed.

@CaerusKaru
Copy link
Author

@eladb There is more we can do in terms of compartmentalizing by creating macros, but really it's up to you to determine the ergonomics you want. So far my goal has just been to get the project "working" in an MVP state. How you want it to work after that is really just gravy now that it does work.

@eladb
Copy link
Contributor

eladb commented Mar 23, 2020

Yes I understand. Let me try to play with this a little to see if I can get to the ergonomics I want

@eladb
Copy link
Contributor

eladb commented May 14, 2020

@CaerusKaru I think I am going to close this one for the time being. I don't think I have the time in the next few months to spend on getting this to a place that I would feel comfortable merging. @iliapolo would you be interested to look into this perhaps?

@eladb eladb closed this May 14, 2020
@eladb
Copy link
Contributor

eladb commented May 14, 2020

@CaerusKaru I really appreciate all the effort!

@iliapolo
Copy link
Member

@eladb definitely. I’ll see if I can pick this up.

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.

3 participants