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: detach src.gen/ from monorepo #5667

Merged
merged 3 commits into from
Sep 27, 2024
Merged

Conversation

hayemaxi
Copy link
Contributor

@hayemaxi hayemaxi commented Sep 25, 2024

Theory: dependabot is still trying to update src.gen/ dependencies because they are managed by the root monorepo. So technically, there is no way to avoid updating src.gen's package.json because the root package.json is linked to it and impacted by dependabot changes for subproject dependencies.

Let's make its own "subrepo" and see if we can get dependabot to ignore it finally.


License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@hayemaxi hayemaxi requested a review from a team as a code owner September 25, 2024 16:34
@hayemaxi hayemaxi marked this pull request as draft September 25, 2024 17:14
@hayemaxi hayemaxi force-pushed the dependabot3 branch 2 times, most recently from e044bbf to 78840bc Compare September 26, 2024 16:52
Theory: dependabot is still trying to update src.gen/ dependencies because they are managed by the root monorepo. So technically, there is no way to avoid updating src.gen's package.json because the root package.json is linked to it and impacted.

Let's make its own "subrepo" and see if we can get dependabot to ignore it finally.
@hayemaxi hayemaxi marked this pull request as ready for review September 26, 2024 17:30
Copy link
Contributor

@justinmk3 justinmk3 left a comment

Choose a reason for hiding this comment

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

This is worth a try as a data point. But the details are complex enough that, even if it works, it might be justified to either:

At this point we have almost no choice.

// vsce runs this command by default.
// This can result in errors that originate from configuration in the src.gen/ directory, but that directory
// cannot be managed directly since it is autogenerated code.
// This section runs the validation command manually and ignores errors only related to src.gen/
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 logic intended to actually tell us if dependabot is trying to recurse into src.gen? Or is it still expected that it will?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its unrelated to dependabot. We are basically just replacing an internal validation step of vsce package with our own that will not throw an error on issues with src.gen/. Without this, splitting off src.gen from the root repo results in a dependency error during packaging based on something in src.gen/node_modules. The extension appears to work normally with this dependency error:

npm ERR! invalid: [email protected] /Volumes/workplace/aws-toolkit-vscode/src.gen/@amzn/codewhisperer-streaming/node_modules/downlevel-dts/node_modules/typescript

After this change I expect it to continue to recurse into src.gen/, but the new ignore rules in dependabot.yml should auto-close PRs that modify it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Without this, splitting off src.gen from the root repo results in a dependency error during packaging based on something in src.gen/node_modules.

That one sentence is most helpful for me, perhaps should be (first) in the code comment.

Does this add fragility to the build? Should we declare bankrupty and fully separate src.gen (whether by branch, subdir, or npm package)?

Copy link
Contributor Author

@hayemaxi hayemaxi Sep 27, 2024

Choose a reason for hiding this comment

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

Does this add fragility to the build? Should we declare bankrupty and fully separate src.gen (whether by branch, subdir, or npm package)?

Obviously it adds some complexity but I wouldn't say its fragile. I don't think its worth the effort to separate src.gen/ if this works unless it causes problems later on.

Update: My initial interpretation of microsoft/vscode-vsce#439 was incorrect. It seems that we don't need vsce's dependency checks at all since we are bundling the extension with webpack. So --no-dependency should be on by default for us and we don't need this custom checking logic that emulates the dependency checks.

Let's try this. If dependabot continues to give us problems we will have to resort to separating it as you suggested.

Copy link
Contributor

@justinmk3 justinmk3 left a comment

Choose a reason for hiding this comment

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

Nice!

@hayemaxi hayemaxi merged commit 7f3f45c into aws:master Sep 27, 2024
21 of 24 checks passed
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