feat: Refactor generator inputs, and first pass at a container interface #13896

jskeet · 2024-11-28T18:48:15Z

No description provided.

jskeet · 2024-12-06T12:47:16Z

@amanda-tarafa: This is far enough along now that it's worth reviewing. I'd encourage you to review one commit at a time, and then note how far you've got (to avoid having to re-review). I don't anticipate modifying earlier commits any more - I suspect it'll be most productive to make changes as new commits.

Work still to do is noted in a second tab in the internal document.

generator-input/tweaks/Google.Cloud.Bigtable.V2/postgeneration.sh

amanda-tarafa · 2024-12-09T07:26:41Z

generator-input/tweaks/Google.Cloud.Compute.V1/postgeneration.sh

+# TODO: Use the appropriate output directory
+
+GENERATOR_OUTPUT=../../..
+COMPUTE_OUTPUT=$GENERATOR_OUTPUT/apis/Google.Cloud.Compute.V1


Leaving this comment here so I don't forget to ask about this:

Since we'll have <output>/apis I'm guessing each language can have whatever structure they need under <output>, right? Which means the CLI can make no assumptions about the structure of <output>?

Basically output is intended to overlay on top of the repo root - the CLI will (currently) make no assumptions about the structure of the repo other than the generator-input directory and a file specifying the container to use.

We may revisit this later, if we want all generated files to live until a single output directory (which then doesn't have any non-generated files in).

amanda-tarafa · 2024-12-09T07:52:03Z

generator-input/renovate-template.json

@@ -0,0 +1,74 @@
+{


Why is this here?

The file itself? It's an input to the generator, so that the whole file is generated - we update "ignorePaths" based on the APIs we generate.

amanda-tarafa · 2024-12-09T08:17:47Z

generator-input/tweaks/Google.Cloud.Compute.V1/postgeneration.sh

+# Build the production library in a temporary directory so that we don't depend on
+# whether we're within a repo or not.
+rm -rf /tmp/compute
 mkdir -p /tmp/compute

+# Even if we've got handwritten code available, we don't want it for this build.


I don't like this. This code is very concerned with how a tool may use it. It feels like these isolation concerns should be handled by the tool and not by this code?

And we just had a chat about this, and the general case is that this is happening in an empty directory, that is, no handwritten code available. So, why do we have to double guard ourselves against that happening?

That's the general case, but not the local development case. We could potentially make sure that we always generate to an empty output directory (and then copy the results) in which case this could be simplified (and yes, I don't like it either). One downside of that would be slowing down local generation - we used to generate separately and then copy, and generating in-place instead massively improved performance.

I'm adding a TODO here to remind us to think about this more.

I think moving the enum constant generator into the GAPIC generator would probably be the best fix for this, to be honest.

amanda-tarafa · 2024-12-09T08:22:58Z

tools/Google.Cloud.Tools.Common/ApiMetadata.cs

+        /// <summary>
+        /// Projects that exist in a non-predictable fashion.
+        /// Only the suffix is required - so for "Google.Cloud.Storage.V2.IntegrationTests" in "Google.Cloud.Storage.V2", this
+        /// would just have "IntegrationTests". Where the suffix doesn't match, a leading caret (^) is used to indicate this.


Oh, do we actually have "where the suffix doesn't match"?

One that I'm aware of: in Google.Cloud.Logging.Console, there's a SampleApp directory. It's deliberately not Google.Cloud.Logging.Console.SampleApp, as we don't want namespace defaulting to change what we import.

tools/Google.Cloud.Tools.ReleaseManager/NonSourceGenerator.cs

amanda-tarafa · 2024-12-09T09:04:31Z

tools/Google.Cloud.Tools.ReleaseManager/NonSourceGenerator.cs

+        JArray ignorePaths = (JArray) jobj["ignorePaths"];
+        foreach (var api in _catalog.Apis)
+        {
+            ignorePaths.Add($"apis/{api.Id}/{api.Id}/**");


Duh, I left a comment somewhere about why renovate was in input.

Maybe there's a way to configure renovate so that we don't have to be explicit about each folder we want to skip? Maybe we can just add the ones we want renovate to look at.

Have added a TODO for this. Now that we're using Directory.Packages.props, we might not need to skip at all.

amanda-tarafa · 2024-12-09T09:12:39Z

tools/Google.Cloud.Tools.ReleaseManager/UpdateProjectPropertiesCommand.cs

+
+public sealed class UpdateProjectPropertiesCommand : CommandBase
+{
+    public UpdateProjectPropertiesCommand()


This is to run whenever we delete a "special" project then right?

Maybe we can have a github action run this on submit, or a check based on this. Similar to how we check "generate projects".

It's also what we'd run when adding a project, I think.

But yes, a github action wouldn't be a bad idea. Will add a TODO.

amanda-tarafa · 2024-12-09T09:17:21Z

generator-input/tweaks/Directory.Packages.props

@@ -0,0 +1,29 @@
+<Project>


Yikes, what makes certain these are the same?

Can't you have the root Directory.Packages.props and global.json point to these somehow?

I don't have a plan right now. We could at least have some validation step (e.g. a GitHub action).

Mind you, to some extent it doesn't really matter if they're not aligned (and I'd expect there to be Roslyn-based packages in here that aren't in the root one). Let's chat about this more though.

amanda-tarafa · 2024-12-09T09:20:45Z

tools/Google.Cloud.Tools.ReleaseManager/NonSourceGenerator.cs

+    // we get consistent project IDs (hashed from the project name).
+    // This also means we don't need the project files to exist before we
+    // create the solution file (as they may be handwritten projects).
+    // Currently this is really ugly - it will be a lot cleaner with slnx files.


Oh but so much better than the ghost project files. I prefer this.

(Some scripts will need to be edited due to the change in location.)

(The scripts still need editing.)

…feas.V1

…input/tweaks

…ator-input/tweaks

This also adjusts the post-generation steps to make sure they work without any current handwritten production code. (In other words, we should be able to generate into an entirely empty directory, and the result should be the same as is in the repo, for all the files that end up being generated.) There's still work to do around project and solution files.

…m generator input This includes moving apis.json into generator-input. Changes compared with the status quo: - Coverage XML files are no longer generated - Solution files won't contain project references at the moment - Documentation stubs won't be generated - Either this will become part of the onboarding step, or we'll create one temporarily during documentation generation if the file is absent at that point - We generate test/snippet project files for OsLogin.Common, but there are no C# files. I believe that should be okay, but it's a change.

…r code Code in the generator-input directory needs to be runnable even if it's separate from the rest of the repo.

…generator-input

This allows us to generate solution files predictably - whereas running "dotnet sln add" generates a new project GUID each time.

We should assume we can write to /tmp or equivalent, but not to the current working directory

The "tweaks" should assume they can modify googleapis - the calling code should take a copy beforehand if necessary.

This "supports" generate and clean as subcommands, but both are unimplemented.

Additionally, use a factory method to create an in-place NonSourceGenerator. We'll want further refactoring later, but this is at least better.

This allows us to determine the root directory when building a Docker image.

… OwlBot

(We want to regenerate project files etc, even if we don't have a protoc plugin.)

…eans We have some code which *is* generated code, but not generated in a regular way - e.g. conformance tests, common resource names, BigQuery resource classes. Those are all updated by running their generators manually, so shouldn't be cleaned here.

This is the result of regenerating all APIs. (There's an uncommitted Artifact Registry change due to duplicate JSON files, which can be fixed separately.)

jskeet · 2024-12-10T11:14:16Z

I believe the test failure is because we're missing the Grafeas project in the solution file. Given that we want project references anyway, I'll just amend the solution generator for that (and check that it actually improves things anyway).

…rences This solves the client creating testing issue for Google.Cloud.DevTools.ContainerAnalysis.V1, as well as helping for projects such as Spanner.Data. (Regeneration in the next commit.)

amanda-tarafa

Just a few nits, this looks good. I'm fine with merging.

amanda-tarafa · 2024-12-10T17:31:09Z

tools/Google.Cloud.Tools.ReleaseManager/ContainerCommand.cs

+        var options = new Dictionary<string, string>();
+        foreach (var arg in args.Skip(1))
+        {
+            if (!arg.StartsWith("--") || !arg.Contains('='))


nit, but: should this be &&?

Nope - it's an error if either it doesn't start with -- or it doesn't contain =.

Could change it to: if (!(arg.StartsWith("--") && arg.Contains('='))) if that would be clearer.

amanda-tarafa · 2024-12-10T17:36:57Z

tools/Google.Cloud.Tools.ReleaseManager/ContainerCommands/CleanCommand.cs

+            // Source code
+            DeleteAll(Directory.EnumerateFiles(layout.SourceDirectory, "*.g.cs", SearchOption.AllDirectories));
+            // Snippet metadata
+            DeleteAll(Directory.EnumerateFiles(Path.Combine(layout.SourceDirectory, $"{id}.GeneratedSnippets"), "*.json"));


nit: But maybe you can just delete de GeneratedSnippets folder entirely.

I wondered about that, but felt it was risky in terms of potentially accidentally deleting handwritten code. While it would be weird for GeneratedSnippets to have non-generated code, I wouldn't be hugely surprised if it happened...

amanda-tarafa · 2024-12-10T17:43:19Z

tools/Google.Cloud.Tools.ReleaseManager/ContainerCommands/GenerateCommand.cs

+/// - api-root: effectively the googleapis directory. We take a copy of this before running generation, unless we're running raw generation.
+/// - generator-input: optional, when omitted, run "raw generation"
+/// - output: root folder for result; required, must exist
+/// - api-path: e.g. google/cloud/functions/v2


Maybe add: "relative to api-root"

amanda-tarafa · 2024-12-10T17:45:29Z

tools/Google.Cloud.Tools.ReleaseManager/ContainerCommands/GenerateCommand.cs

+                var id = catalog.Apis.FirstOrDefault(api => api.ProtoPath == apiPath)?.Id;
+                if (id is null)
+                {
+                    // TODO: Optionally, create a temporary configuration.


Possibly using "oboard" command but without saving the result.

Maybe. I'm really not sure at the moment, to be honest. It's possible that in-memory will be a better approach, so that we can make generator-input readonly...

jskeet requested a review from amanda-tarafa November 28, 2024 18:48

jskeet force-pushed the generator-input branch 2 times, most recently from 9f0b98c to 1fd2fe3 Compare December 6, 2024 12:44

amanda-tarafa reviewed Dec 9, 2024

View reviewed changes

jskeet marked this pull request as ready for review December 9, 2024 16:37

jskeet requested review from a team as code owners December 9, 2024 16:37

jskeet added 20 commits December 10, 2024 07:19

tools: Expect project augmentation files from generator-input/tweaks

22cfc00

chore: Move project augmentation files

f77f364

tools: Expect pre/post-generation scripts to be in the tweaks directory

b9081e6

(Some scripts will need to be edited due to the change in location.)

chore: Move pre/post-generation scripts

b3ef551

(The scripts still need editing.)

chore: Fix postgeneration scripts to adapt to the new location

7a9afa9

chore: Move Grafeas.V1.FixGeneratedCode to generator-input/tweaks/Gra…

088a842

…feas.V1

chore: Move Google.Cloud.Compute.V1 postgeneration code to generator-…

0f42bfd

…input/tweaks

chore: Move postgeneration code for Google.Cloud.Bigtable.V2 to gener…

63346bf

…ator-input/tweaks

chore: Add existing "extra" projects to the API catalog

ff8d342

chore: Delete coverage files

72be6e1

chore: Add global.json and Directory.Packages.props for post-processo…

404ebb1

…r code Code in the generator-input directory needs to be runnable even if it's separate from the rest of the repo.

chore: Update apis.json validation workflow

4b10422

tools: Move resource configurations (general and API-specific) under …

3cef65b

…generator-input

tools: Generate solution files "manually"

bc9523c

This allows us to generate solution files predictably - whereas running "dotnet sln add" generates a new project GUID each time.

tools: Avoid using a relative temporary directory when generating

0631fb6

We should assume we can write to /tmp or equivalent, but not to the current working directory

chore: Stop using git to undo changes in googleapis

4a7f0dc

The "tweaks" should assume they can modify googleapis - the calling code should take a copy beforehand if necessary.

tools: Initial framework for a container command

c281977

This "supports" generate and clean as subcommands, but both are unimplemented.

refactor: Load the API catalog in the NonSourceGenerator constructor

93259d2

Additionally, use a factory method to create an in-place NonSourceGenerator. We'll want further refactoring later, but this is at least better.

jskeet added 10 commits December 10, 2024 07:20

chore: Comments and minor fixes from code review

72c2003

tools: First pass at a "clean" container command

caf85bc

chore: use toolversions.sh to determine the repo root directory

b95af7a

This allows us to determine the root directory when building a Docker image.

tools: First pass at a "generate" container command

e7bdf5f

fix: Use the input/output directory when detecting postgeneration for…

1893315

… OwlBot

feat: First pass at a Docker container

5e21be8

fix: Don't assume that the generated snippets directory exists

ea9cab6

fix: Fix generated solution files

44f75ea

tools: Always generate non-source files in GenerateApis

840051c

(We want to regenerate project files etc, even if we don't have a protoc plugin.)

chore: Move logging of API generation to include non-micro/proto APIs

58deb03

jskeet force-pushed the generator-input branch from 79d764a to 91033a5 Compare December 10, 2024 07:21

jskeet force-pushed the generator-input branch from 91033a5 to 55786cb Compare December 10, 2024 07:49

chore: Regenerate all solution files and add "empty" project files

6e3a438

This is the result of regenerating all APIs. (There's an uncommitted Artifact Registry change due to duplicate JSON files, which can be fixed separately.)

jskeet changed the title ~~Prototyping of a generator-input directory~~ feat: Refactor generator inputs, and first pass at a container interface Dec 10, 2024

jskeet added 2 commits December 10, 2024 11:32

tools: Add solution references to projects where we have project refe…

5675460

…rences This solves the client creating testing issue for Google.Cloud.DevTools.ContainerAnalysis.V1, as well as helping for projects such as Spanner.Data. (Regeneration in the next commit.)

chore: Regenerate solution files with more projects

76a44b6

amanda-tarafa approved these changes Dec 10, 2024

View reviewed changes

chore: Clarify the api-path option in the generate container command

e8ca981

jskeet merged commit 13dd39d into googleapis:main Dec 10, 2024
9 checks passed

jskeet deleted the generator-input branch December 10, 2024 18:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Refactor generator inputs, and first pass at a container interface #13896

feat: Refactor generator inputs, and first pass at a container interface #13896

jskeet commented Nov 28, 2024

jskeet commented Dec 6, 2024

amanda-tarafa Dec 9, 2024

jskeet Dec 9, 2024

amanda-tarafa Dec 9, 2024

jskeet Dec 9, 2024

amanda-tarafa Dec 9, 2024

jskeet Dec 9, 2024

jskeet Dec 9, 2024

jskeet Dec 9, 2024

amanda-tarafa Dec 9, 2024

jskeet Dec 9, 2024

amanda-tarafa Dec 9, 2024

jskeet Dec 9, 2024

amanda-tarafa Dec 9, 2024

amanda-tarafa Dec 9, 2024

jskeet Dec 9, 2024

amanda-tarafa Dec 9, 2024

jskeet Dec 9, 2024

amanda-tarafa Dec 9, 2024

jskeet commented Dec 10, 2024

amanda-tarafa left a comment

amanda-tarafa Dec 10, 2024

jskeet Dec 10, 2024

amanda-tarafa Dec 10, 2024

jskeet Dec 10, 2024

amanda-tarafa Dec 10, 2024

jskeet Dec 10, 2024

amanda-tarafa Dec 10, 2024

jskeet Dec 10, 2024

feat: Refactor generator inputs, and first pass at a container interface #13896

feat: Refactor generator inputs, and first pass at a container interface #13896

Conversation

jskeet commented Nov 28, 2024

jskeet commented Dec 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet commented Dec 10, 2024

amanda-tarafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment