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

process: sync commits from Aspect-internal silo #772

Merged
merged 15 commits into from
Nov 7, 2024

Conversation

Copy link

aspect-workflows bot commented Nov 7, 2024

Test

All tests were cache hits

208 tests (100.0%) were fully cached saving 9m 1s.


Buildifier      Format

…// comments (#7163)

Comments such as `////////////////// copyright` will pass the
`strings.HasPrefix(s, "///")` but will correctly fail the regex and
should not spam stdout.

---

### 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

Comments similar to typescript triple-comment references should not
output errors during `aspect configure`.

### Test plan

- Covered by existing test cases

GitOrigin-RevId: 5921cfc2ba722a8375051c86b18288e1fc256452
Technically `string([]byte{})` clones the bytes array because the string
version is immutable and can't reference the array which could be
modified.

In most cases converting `os.ReadFile` results to a string is just for
convenience and should be avoided until necessary.

---

### 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

Minor reduction in memory allocation during `aspect configure`,
especially parsing of source code.

### Test plan

- Covered by existing test cases

GitOrigin-RevId: 7f0079d3b0f7bf82fc858f09620b0071ba3e3507
### 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

Minor reduction in memory allocation during `aspect configure`

### Test plan

- Covered by existing test cases

GitOrigin-RevId: 69285476ec9e6db5243affc66ef2ec00a733d8cb
…rsing (#7164)

The regex compilation in `treesitter/filters.go` is noticeable when
profiling.

---

### 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

Cache parsing of regex statements for `aspect configure` source code
parsing.

### Test plan

- Covered by existing test cases

GitOrigin-RevId: a372b0b890860c0066ed161d5fcef99eb6d2dbb0
When extending tsconfig such as `@corp/foo` we should not check for
files of that name on the fs.

---

### 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

Prevent extra fs lookup for `aspect configure` tsconfig `extends`.

### Test plan

- Covered by existing test cases

GitOrigin-RevId: 50f14ffa222587975a4812ce59a0767f1a3ab3c3
All of our tree-sitter queries are cached, this adds some additional
metadata caching to prevent recomputing on every query execution and in
some cases every query match.

---

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

- 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

Caching of `aspect configure` tree-sitter query metadata across query
executions.

### Test plan

- Covered by existing test cases

GitOrigin-RevId: 85d25355eded802cdb408d0c24364b40d1062b08
We should not be using `os.Stat` to check if files exist. It is very
expensive and gazelle has already read the full list of entries for the
directory.

The gazelle patch exposes that list. This is similar to
bazel-contrib/bazel-gazelle#1737 which was done
at airtable for similar reasons.

On one major repo this reduces the time by almost 2s which is ~15% and
well worth the patch imo.

---

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

- 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 fs io of `aspect configure` during fs traversal.

### Test plan

- Covered by existing test cases

GitOrigin-RevId: b17d52b2723d3b7e4d5ee33fc294f491062ded5a
When trying to migrate rules_go to bzlmod an external go mod somewhere
ends up with an older version of these for some reason, so may as well
just upgrade if we can? 🤷

---

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

### Test plan

- Covered by existing test cases

GitOrigin-RevId: 7f198dba436684901c769de4c4e878903df1e06f
…x (#7201)

See
bazel-contrib/bazel-gazelle@8c64e02

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

### Test plan

- Covered by existing test cases

GitOrigin-RevId: 3eed1cca3bdcf3e9408b27acb1eca06157fe70c0
…o (#7116)

Vendor the bazelisk/core.go file with minimal changes. Some of the
vendored code with a diff is still within bazelisk.go and can maybe be
refactored later

---

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

### Test plan

- Covered by existing test cases
- Manual testing; please provide instructions so we can reproduce: run
the pro cli

GitOrigin-RevId: 0beca33dce43bf7c51c962beef2851ca3a72ad63
The rules_go gazelle integration changed in bzlmod such that it now
requires resolving the
[bazel_gazelle_go_repository_config](https://github.com/bazel-contrib/bazel-gazelle/blob/v0.39.1/internal/bzlmod/go_deps.bzl#L653-L672)
repository which contains a cache-able list of the `go_repository`
target data (not the actual targets, just data about them).

With regular gazelle from a `gazelle_binary` target the
`@bazel_gazelle_go_repository_config` repo path is resolved using [a
rule label
attribute](https://github.com/bazel-contrib/bazel-gazelle/blob/v0.39.1/def.bzl#L101-L104)
and [this
hack](https://github.com/bazel-contrib/bazel-gazelle/blob/v0.39.1/internal/go_repository.bzl#L313-L321).
However when we are running gazelle within `aspect configure` we need an
alternate way of resolving this path.

This PR does something similar to the [the go_repository
hack](https://github.com/bazel-contrib/bazel-gazelle/blob/v0.39.1/internal/go_repository.bzl#L313-L321),
except we must launch `bazel info output_base` to get the external
directory.

Fix #760

---

### 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

The go language in `aspect configure` will now work when rules_go is
under bzlmod.

### Test plan

- Manual testing; please provide instructions so we can reproduce: run
the cli binary on a repo where rules_go is loaded with bzlmod, for
example https://github.com/r0bobo/aspect-cli-issue-760-reproduce

GitOrigin-RevId: aded45cc9e6566447383b193a201255e55e90730
…nfig.json value (#7257)

This was not set at all previously.

---

### 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

The `ts_project` `preserve_jsx` attribute is now generated by `aspect
configure`.

### Test plan

- Covered by existing test cases
- New test cases added

GitOrigin-RevId: 8f3a806c83de385cfee4be427df500684e78aa02
…s (#7260)

If the js gazelle extension did not generate a target it should not
crash and standard imports of any files in `js_library(srcs)` should
still work.

---

### 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

`aspect configure` orion generated targets of rule kinds known to the
configure js language should work across languages.

### Test plan

- Covered by existing test cases
- New test cases added

GitOrigin-RevId: 3312e52f2fd4764df1fbaa05f0ae6ac1cc489fbc
This would have been useful recently when debugging tsconfig issues.

---

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

### Test plan

- Covered by existing test cases

GitOrigin-RevId: fae3a0d76857bdaf185fc30d7953c7da538521ba
@jbedard jbedard force-pushed the 788EA150BB0FAAB4A3FF81F26DCBE189 branch from 6179a50 to d3e076e Compare November 7, 2024 21:52
@jbedard jbedard requested a review from alexeagle November 7, 2024 21:55
@jbedard jbedard enabled auto-merge (rebase) November 7, 2024 22:14
@jbedard jbedard merged commit 2a2625b into main Nov 7, 2024
10 checks passed
@jbedard jbedard deleted the 788EA150BB0FAAB4A3FF81F26DCBE189 branch November 7, 2024 22:24
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.

2 participants