Skip to content

process: sync commits from Aspect-internal silo #789

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 17 commits into from
Closed

process: sync commits from Aspect-internal silo #789

wants to merge 17 commits into from

Conversation

gregmagolan
Copy link
Member

jbedard and others added 17 commits December 14, 2024 23:04
@jbedard @gregmagolan
chore: remove docs/help from cli copybara config (#7462)
0efa45e 
This was removed from silo? So it causes an error when copybara runs
now...

---

### Changes are visible to end-users: no

### Test plan

- Manual testing; please provide instructions so we can reproduce: run
copybara

GitOrigin-RevId: 67e6fe8d8b40a818e984fdc4e62c4a557cbc8b82
@gregmagolan
fix: when bootstrapping plugins build with --remote_download_outputs=…
7c9f1b8 
…toplevel (#7472)

This is needed if build without the bytes is on. Same as #7471 but for
the plugin bootstrap.

---

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases

GitOrigin-RevId: b77cc4ffd668268f09bf1261777125570d3fb891
@jbedard @gregmagolan
docs(cli): fix broken link in read me (#7463)
1e62c3a 
Synced from #779 by
ChrisChinchilla:
docs: Fix broken link in read me

---

Fix broken link in readme file.

### Changes are visible to end-users: no

Closes #779

Co-authored-by: Chris Chinchilla <chris@aspect.build>
GitOrigin-RevId: 1a42e0331f2b210f8cb50c27edd7957db74ecc79
@JesseTatasciore @gregmagolan
feat(CLI): Allow for salting on the outputs verb (#7415)
675961d 
Allow for a salt value to be passed into the outputs verbs hashing generator

---

### Changes are visible to end-users: yes

- Searched for relevant documentation and updated as needed: no
- Breaking change (forces users to change their own code or config): no
- Suggested release notes appear below: yes

CLI: To add a salt value to hashes generated from the `outputs` verb, specify the `--hash_salt` flag when calling `outputs`

### Test plan

- Covered by existing test cases
- New test cases added
- Manual testing; please provide instructions so we can reproduce:

Just ran the commands with the flags and ran it with following changes for rosetta that use this flag

GitOrigin-RevId: 1d44a3d07b9e797aaa52e86cc2a722fe213f3136
@jbedard @walkerburgin @gregmagolan
fix(cli): reflect the tsconfig "composite" and "incremental" setting …
4050a26 
…into ts_project() rules (#7460)

Synced from #780 by
walkerburgin:
Reflect tsconfig `composite` and `incremental` to `ts_project()`

---

I noticed that `bazel configure` is creating `BUILD.bazel` files that
would result in the following error:

```
ERROR: ts_project rule @@//pkgs/foo-lib:typescript was configured with attributes that don't match the tsconfig
 - attribute composite=false does not match compilerOptions.composite=true
 - attribute incremental=false does not match compilerOptions.incremental=true
You can automatically fix this by running:
    npx @bazel/buildozer 'set composite True' 'set incremental True' @@//pkgs/foo-lib:typescript
Or to suppress this error, either:
 - pass --norun_validations to Bazel to turn off the feature completely, or
 - disable validation for this target by running:
    npx @bazel/buildozer 'set validate False' @@//pkgs/foo-lib:typescript
```

It would be nice for these `compilerOptions` to be automatically
mirrored into the `ts_project()`.

---

### Changes are visible to end-users: no

### Test plan

- New test cases added

Closes #780

Co-authored-by: Walker Burgin <walkerburgin@gmail.com>
GitOrigin-RevId: 7897b6f963be24baa3833bf9c39c6287ac8fd383
@jbedard @henkerik @gregmagolan
feat(cli): custom tsconfig.json file name (#7484)
17394c6 
Synced from #784 by
henkerik:

---

### Changes are visible to end-users: yes

- Searched for relevant documentation and updated as needed: no
- Breaking change (forces users to change their own code or config): no
- Suggested release notes appear below: yes

```
# gazelle:js_tsconfig_file _tsconfig_json_

Path to the `tsconfig.json` file, in case your TSConfig file has a different file name
```

### Test plan

- New test cases added: `bazel test
//gazelle/js:tsconfig_custom_file_name_test`

Closes #784

Co-authored-by: Henk Erik van der Hoek <mail@henkerikvanderhoek.nl>
GitOrigin-RevId: b773c2626f32ffa826f85ed52e31472acad93a7c
@jbedard @gregmagolan
perf(cli): parse and cache orion queries on declaration (#7287)
c83056a 
.. instead of re-checking the cache on every execution of a query. This
reduces locking for cache check+write on every query execution

---

### Changes are visible to end-users: yes

- Searched for relevant documentation and updated as needed: yes
- Breaking change (forces users to change their own code or config): no
- Suggested release notes appear below: yes

Reduce locking during concurrent orion query execution.

### Test plan

- Covered by existing test cases

GitOrigin-RevId: 88b1a8b419df7a1081a734f7881c15c9442bac36
@jbedard @gregmagolan
fix(cli): add generated ts sources to ts_project(srcs) (#7486)
af29eba 
Close #782

---

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases
- New test cases added

GitOrigin-RevId: 0155ebe4fc0d549340257df6c73d12e6de91ac39
@mrmeku @gregmagolan
chore(cli): Fix spelling mistake (#7515)
14284a2 
---

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases

GitOrigin-RevId: 4d0800e803a4f11720005fbe8657bce9acb93222
@gregmagolan
refactor: remove ability to build Workflows plugin from target (#7577)
dc6ee2d 
Follow-up to #7558.

Building CLI and plugins from a bazel target is additional complexity
that is not worth supporting. It doesn't work great internally and a
customer (Airtable) tried using it for plugins (because they could) and
it didn't work great for them either.

Airtable ended up breaking the glass and building the plugin out of band
before calling the workflows task. This PR sets up the same pattern for
silo. After this lands, I'll remove the build from target functionality
in both rosetta (to build the CLI) and in the CLI (to build a plugin)
since building out of band is strictly better.

---

### Changes are visible to end-users: yes

- Searched for relevant documentation and updated as needed: yes
- Breaking change (forces users to change their own code or config): yes
- Suggested release notes appear below: yes

BREAKING CHANGE

Aspect CLI no longer supported building and running a Workflows plugin
from a target. Instead it is recommended to run it from a built binary
during plugin development. For example,
https://github.com/aspect-build/plugin-fix-visibility/blob/a8acbbd2bce5a6e5cf6990050e438acd505dafbc/.aspect/cli/config.yaml#L8.

### Test plan

- Covered by existing test cases

GitOrigin-RevId: b2ceb89caf4eb982313f8ffd8686d5eab437aef7
@gregmagolan
chore: remove old unused Aspect CLI plugin API versions (#7579)
4a8aec6 
These are taking up space.

---

### Changes are visible to end-users: no

### Test plan

- Covered by existing test cases

GitOrigin-RevId: 0ddf73b76ee094fc603e7ab34e298f29adc90789
@jbedard @walkerburgin @gregmagolan
fix(cli): handle non @types imports from tsconfig `compilerOptions.…
5bc7c9c 
…types` (#7548)

Synced from #785 by
walkerburgin:

---

The TypeScript docs for `compilerOptions.types` make it sound like
`compilerOptions.types` only applies to `@types` packages, but in
practice it can also be used to include types from non `@types` in the
global scope. For example, `cypress` recommends this in their
[docs](https://docs.cypress.io/app/tooling/typescript-support#Configure-tsconfigjson):

```json
{
  "compilerOptions": {
    "target": "es5",
    "lib": ["es5", "dom"],
    "types": ["cypress", "node"]
  },
  "include": ["**/*.ts"]
}
```

From what I can tell, if you don't specify `compilerOptions.types`,
TypeScript will **not** automatically include types from a visible non
`@types` package like `cypress` (`describe`, `beforeEach`, etc). But you
can include non `@types` packages in `compilerOptions.types` to allow
list them into the global scope.

---

### Changes are visible to end-users: no

### Test plan
I updated the existing test case for `compilerOptions.types`.

- Covered by existing test cases
- New test cases added

Closes #785

Co-authored-by: Walker Burgin <wburgin@anduril.com>
GitOrigin-RevId: 4321e2b70b9e8d1f68fff219b42cae02a5d3205a
@jbedard @gregmagolan
fix(cli): support orion properties with no declared default (#7539)
51fa301 
Plugin properties should not require explicit defaults.

---

### Changes are visible to end-users: yes

- Searched for relevant documentation and updated as needed: yes
- Breaking change (forces users to change their own code or config): no
- Suggested release notes appear below: yes

Orion plugin property declarations no longer require a default.

### Test plan

- Covered by existing test cases
- New test cases added

GitOrigin-RevId: 463a7bd63fb13ea265eb4fc923fc0f3f617d0865
@jbedard @gregmagolan
test(cli): support imports to generated files in rootDirs (#7509)
8871db5 
Close #783

Was fixed by
aspect-build/silo@0155ebe

---

### Changes are visible to end-users: no

### Test plan

- New test cases added

GitOrigin-RevId: 825890f53c11797b3f1e771a53d8c769e6d51dbb
@jbedard @gregmagolan
fix(cli): support js deps via package.json types (#7589)
d18034d 
The package.json `types|typings` field should be considered a dependency
just like the `main` field.

---

### Changes are visible to end-users: yes

- Searched for relevant documentation and updated as needed: yes
- Breaking change (forces users to change their own code or config): no
- Suggested release notes appear below: yes

Support `aspect configure` detecting dependencies from `package.json`
`types|typings` fields.

### Test plan

- Covered by existing test cases
- New test cases added

GitOrigin-RevId: f0198476cbbe8f755510e4b0992b020fdcf3059c
@gregmagolan
fix: improve performance of BES handling for Aspect CLI & Workflows p…
764bd7f 
…lugin (#7585)

Resolves performance issues with BES event processing in the Aspect CLI
and in the Workflows plugin.

The current implementation was the receiving a single bes event,
forwarding it to Workflows plugin, waiting for response from plugin and
finally sending ack back to the Bazel server for that event before
starting the recv for the next bes event. This was created a bottleneck
since events could not be received and processed and ack'd in parallel.

The fix here is to leverage multiple go routines in the BES backend:

- 1 to recv the grpc BES stream events from the Bazel server
- 10 to forward to Workflows plugin with BEPEventCallback API calls
- 1 to ack the grpc BES events so the Bazel server keeps sending as we
are receiving and processing

Plugins need to be opted in the new behavior via
`.aspect/cli/config.yaml` setting. Plugins that opt-in will need to
handle multi-threaded calls on that method and potentially out of order
events. For this reason, this performance bump is being landed as an
opt-in so customers with custom plugins can update their custom plugin
code accordingly before turning it on.

Airtable will be the first to opt-in after this lands since they are
suffering the most from the bug (60+ seconds of BES processing after the
bazel build completes on some of their builds).

---

### Changes are visible to end-users: yes

- Searched for relevant documentation and updated as needed: yes
- Breaking change (forces users to change their own code or config): no
- Suggested release notes appear below: yes

Workflows now supports faster build event transmission to Aspect CLI
plugins by sending up to 10 build events in parallel via 10 concurrent
BEPEventCallback calls. Plugins can opt-in to faster build event
transmission by setting `multi_threaded_build_events: true` in their
configuration in the `.aspect/cli/config.yaml` configuration file.
Plugins that opt-in must handle multi-threaded BEPEventCallback. If
plugins require build events to be processed in order they should
buffering out-of-order build events and processing the build events in
order of sequence number. The sequence number of a build event can be
obtained via a new 2nd parameter of the BEPEventCallback function:
`BEPEventCallback(event *buildeventstream.BuildEvent, sequenceNumber
int64)`

### Test plan

- Covered by existing test cases

GitOrigin-RevId: e4fa5c4d4d1866b282b56fdbc23d8ad5d171c253
@jbedard @walkerburgin @gregmagolan
fix(cli): handle /// <reference lib="..." /> comments (#7605)
e96a193 
Synced from #786 by
walkerburgin:
Handle `/// <reference lib="..." />` comments

---

According to the TypeScript docs
([here](https://www.typescriptlang.org/docs/handbook/triple-slash-directives.html#-reference-lib-)):

> This directive allows a file to explicitly include an existing
built-in lib file.
>
> Built-in lib files are referenced in the same fashion as the
[lib](https://www.typescriptlang.org/tsconfig#lib) compiler option in
tsconfig.json (e.g. use lib="es2015" and not lib="lib.es2015.d.ts",
etc.).

I think `bazel configure` is currently attempting to interpret them as
types imports, leading to errors like this:

```
Import "webworker" from "pkgs/foo-lib/src/worker.ts" is an unknown dependency. Possible solutions:
	1. Instruct Gazelle to resolve to a known dependency using a directive:
		# gazelle:resolve [src-lang] js import-string label
		   or
		# gazelle:js_resolve import-string-glob label
	2. Ignore the dependency using the '# gazelle:js_ignore_imports webworker' directive.
	3. Disable Gazelle resolution validation using '# gazelle:js_validate_import_statements off'
```

`pkgs/foo-lib/src/worker.ts`:

```ts
/// <reference lib="webworker" />

postMessage({ message: "Hello, world" });
```

---

### Changes are visible to end-users: yes

<!-- If no, please delete this section. -->

- Searched for relevant documentation and updated as needed: yes
- Breaking change (forces users to change their own code or config): yes
(I guess if somebody was accidentally using `lib` instead of `types`?)
- Suggested release notes appear below: yes

`aspect configure` handles `/// <reference lib="..." />` comments
correctly

### Test plan

<!-- Delete any which do not apply -->

- Covered by existing test cases (I updated the existing test)

Closes #786

Co-authored-by: Walker Burgin <wburgin@anduril.com>
GitOrigin-RevId: 2279a13b8b57373a233ccbd1758ec0595eb682ec
@aspect-workflows Aspect Workflows
Copy link

aspect-workflows bot commented Dec 15, 2024 •
edited
Loading

Test

⚠️ CircleCI build #2822 failed.

//bazel/command_line:write_pb_go failed to build

error loading package 'bazel/command_line': Label '//bazel/ts:defs.bzl' is invalid because 'bazel/ts' is not
a package; perhaps you meant to put the colon here: '//bazel:ts/defs.bzl'?

//bazel/packages_metrics:write_pb_go failed to build

error loading package 'bazel/packages_metrics': Label '//bazel/ts:defs.bzl' is invalid because 'bazel/ts' is
not a package; perhaps you meant to put the colon here: '//bazel:ts/defs.bzl'?

//bazel/query:write_pb_go failed to build

error loading package 'bazel/query': Label '//bazel/ts:defs.bzl' is invalid because 'bazel/ts' is not a
package; perhaps you meant to put the colon here: '//bazel:ts/defs.bzl'?

//bazel/analysis:write_pb_go failed to build

error loading package 'bazel/analysis': Label '//bazel/ts:defs.bzl' is invalid because 'bazel/ts' is not a
package; perhaps you meant to put the colon here: '//bazel:ts/defs.bzl'?

//bazel/flags:write_pb_go failed to build

error loading package 'bazel/flags': Label '//bazel/ts:defs.bzl' is invalid because 'bazel/ts' is not a
package; perhaps you meant to put the colon here: '//bazel:ts/defs.bzl'?

51 other actions failed to build.

💡 To reproduce the build failures, run

bazel build //bazel/command_line:write_pb_go //bazel/packages_metrics:write_pb_go //bazel/query:write_pb_go //bazel/analysis:write_pb_go //bazel/flags:write_pb_go

Buildifier

Buildifier lint check has failed

./bazel/action_cache/BUILD.bazel:5: out-of-order-load: Load statement is out of its lexicographical order. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#out-of-order-load)
./bazel/analysis/BUILD.bazel:5: out-of-order-load: Load statement is out of its lexicographical order. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#out-of-order-load)
./bazel/buildeventstream/BUILD.bazel:5: out-of-order-load: Load statement is out of its lexicographical order. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#out-of-order-load)
./bazel/command_line/BUILD.bazel:5: out-of-order-load: Load statement is out of its lexicographical order. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#out-of-order-load)
./bazel/failure_details/BUILD.bazel:5: out-of-order-load: Load statement is out of its lexicographical order. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#out-of-order-load)
./bazel/flags/BUILD.bazel:5: out-of-order-load: Load statement is out of its lexicographical order. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#out-of-order-load)
./bazel/invocation_policy/BUILD.bazel:5: out-of-order-load: Load statement is out of its lexicographical order. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#out-of-order-load)
./bazel/options/BUILD.bazel:5: out-of-order-load: Load statement is out of its lexicographical order. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#out-of-order-load)
./bazel/packages_metrics/BUILD.bazel:5: out-of-order-load: Load statement is out of its lexicographical order. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#out-of-order-load)
./bazel/query/BUILD.bazel:5: out-of-order-load: Load statement is out of its lexicographical order. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#out-of-order-load)

💡 To reproduce the buildifier failures, run

bazel run //:buildifier.check

Some lint failures can be fixed automatically by running the following while other require manual fixes

bazel run //:buildifier

📝 For more information on how to resolve or suppress specific lint failures, see the Buildifier documentation

Format

Formatting check has failed

💡 Some formatting failures can be fixed automatically by running the command below, while others may require manual fixes

bazel run //:format -- pkg/aspect/outputs/hash.go

ℹ️ A patch file containing the changes has been archived as an artifact of this build

@gregmagolan gregmagolan mentioned this pull request Dec 15, 2024
Merged
@gregmagolan
Copy link
Member Author

@jbedard already did the work to green this up here #788

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gregmagolan @jbedard @JesseTatasciore @mrmeku