Revert "Revert "chore: move codegen logic (#206)"" #209

alex-chew · 2023-03-30T03:00:26Z

See #206

This reverts commit 1080c92.

robin-aws · 2023-03-30T17:08:10Z

LGTM (again) but want to make sure at least one crypto-tools member acks too, given the impact

seebees

LGTM

seebees · 2023-03-30T18:06:37Z

...y-dafny-codegen/src/main/java/software/amazon/polymorph/smithydotnet/DotNetNameResolver.java

+import software.amazon.polymorph.traits.DafnyUtf8BytesTrait;
 import software.amazon.polymorph.traits.LocalServiceTrait;
+import software.amazon.polymorph.traits.PositionalTrait;
+import software.amazon.polymorph.traits.ReferenceTrait;


These kinds of basically whitespace changes are annoying.
It would be wonderful to have a linter/formater for this

Ack, I'll look into auto-formatting for a future PR.

(#209 added it there)

texastony · 2023-03-31T18:55:53Z

.github/workflows/test_models_java_tests.yml

@@ -71,14 +71,14 @@ jobs:
        uses: actions/setup-java@v3
        with:
          distribution: 'corretto'
-          java-version: 8


This is wrong.
Our public Java products MUST support Java 8.
i.e: The Code Generator needs Java 17.
But the Generated Code MUST support and MUST BE tested in Java 8.

Ack. I will fix this and other instances in a follow-up PR.

texastony

This PR was massive.
In addition to moving the entire codegen repo,
it compromised our testing
and refactoring our outstanding PRs to accommodate this refactor is, frankly, an annoying waste of time.

In the future, please attempt to make smaller PRs.

There was no reason, at all, to refactor the Import statements.
But because the import statements were refactored along with all the file movement,
outstanding PRs that actually deliver value must be manually refactored to account for this PR.

texastony · 2023-03-31T19:27:52Z

.github/workflows/test_models_dafny_verification.yml

@@ -64,7 +64,7 @@ jobs:
      - name: Setup Maven Action
        uses: s4u/setup-maven-action@v1.7.0
        with:
-          java-version: 8


This is wrong. This is the Java Version used for installing the Danfy Runtime Jar and the Dafny Java Conversion Library. It MUST be Java 8. Please Revert.
Our public Java products MUST support Java 8.
i.e: The Code Generator needs Java 17.
But the Generated Code MUST support and MUST BE tested in Java 8.

Ack. I will fix this and other instances in a follow-up PR.

Some extra context on this particular instance, for posterity - the setup-maven-action annoyingly overrides the default Java version with whichever java-version specified. That is, if you setup-java for Java 17, and later setup-maven-action for Java 8, then future java and gradle invocations will use Java 8 by default - and gradle doesn't yet know how to automatically locate the Java 17 installation even if the project it's compiling demands Java 17. The change in this PR is the same workaround I applied in #177.

PR to revert/fix Java version changes: #221

texastony · 2023-03-31T19:28:32Z

.github/workflows/test_models_java_tests.yml


      # TODO: This has been added has a workaround for https://issues.apache.org/jira/browse/MNG-7679
      # Remove this when the latest version has been patched
      - name: Setup Maven Action
        uses: s4u/setup-maven-action@v1.7.0
        with:
-          java-version: 8


This is wrong.
Our public Java products MUST support Java 8.
i.e: The Code Generator needs Java 17.
But the Generated Code MUST support and MUST BE tested in Java 8.

Ack, will fix - see details in #209 (comment)

texastony · 2023-03-31T19:29:03Z

.github/workflows/test_models_net_tests.yml

@@ -87,7 +87,7 @@ jobs:
      - name: Setup Maven Action
        uses: s4u/setup-maven-action@v1.7.0
        with:
-          java-version: 8
+          java-version: 17


This is wrong.
This is the Java Version used for installing the Danfy Runtime Jar and the Dafny Java Conversion Library.
It MUST be Java 8. Please Revert.
Our public Java products MUST support Java 8.
i.e: The Code Generator needs Java 17.
But the Generated Code MUST support and MUST BE tested in Java 8.

Ack, will fix - see details in #209 (comment)

texastony · 2023-03-31T19:46:59Z

...gen/src/main/java/software/amazon/polymorph/smithyjava/generator/library/ToDafnyLibrary.java

+import software.amazon.polymorph.traits.DafnyUtf8BytesTrait;
+import software.amazon.polymorph.traits.PositionalTrait;
+import software.amazon.polymorph.utils.ModelUtils;


I am annoyed by almost all of these Import statement changes.
I do not understand the value they add.
Nor do I understand the heuristic used to determine the order of these import statements.

Alphabetically, traits is after smithyjava. As is utils.

Surely this was enough change in this PR as is without these pointless import changes?

Yup I was mildly annoyed too. We should discuss our collective formatting preferences on #211.

To expand on this - I let IntelliJ IDEA handle the refactoring of moving the code, and it performed these import changes according to its default formatting settings, without me noticing. I should've taken a closer look at the diff and reverted them.

texastony · 2023-03-31T19:49:12Z

...rc/main/java/software/amazon/polymorph/smithyjava/generator/library/shims/NativeWrapper.java

+import software.amazon.polymorph.smithyjava.generator.library.ShimLibrary;
+import software.amazon.polymorph.utils.AwsSdkNameResolverHelpers;
+import software.amazon.polymorph.utils.ModelUtils;


Confusion: What is the heuristics for ordering Import statements?
It does not appear to be consistent.

This resulted from IDE auto-formatting - details in #209 (comment)

texastony · 2023-03-31T19:51:06Z

...y-codegen/src/main/java/software/amazon/polymorph/smithyjava/nameresolver/AwsSdkDafnyV1.java


 import software.amazon.smithy.model.Model;
 import software.amazon.smithy.model.shapes.ResourceShape;
 import software.amazon.smithy.model.shapes.ServiceShape;
 import software.amazon.smithy.utils.StringUtils;

 import static software.amazon.polymorph.smithydafny.DafnyNameResolver.traitNameForServiceClient;
-import static software.amazon.polymorph.utils.DafnyNameResolverHelpers.dafnyCompilesExtra_;


Confusion: When is it acceptable to have a static import?
This PR has pointlessly removed quiet a few of them while leaving others,
seemingly at random.

This resulted from IDE auto-formatting - details in #209 (comment)

robin-aws · 2023-03-31T20:23:54Z

This PR was massive. In addition to moving the entire codegen repo, it compromised our testing and refactoring our outstanding PRs to accommodate this refactor is, frankly, an annoying waste of time.

In the future, please attempt to make smaller PRs.

There was no reason, at all, to refactor the Import statements. But because the import statements were refactored along with all the file movement, outstanding PRs that actually deliver value must be manually refactored to account for this PR.

@texastony - I'm very sorry this caused you this much trouble. We had some offline discussions about this change that might not have reached you in time, and I did want to make sure we didn't make it without cryptools awareness so we were careful not to merge without an extra approval (or rather accidentally merged and then reverted :).

I definitely agree in principle that smaller PRs are always better, but in this case I think moving code in chunks would have been a lot of overhead, and it was better to rip the bandaid off quickly. We knew this would cause some PR merge headaches, but is there testing we're not aware of that it also broke?

Your point about the simultaneous import refactorings is valid and other reviewers pointed it out too. We cut #211 to try to avoid a future occurrence.

We'll respond to your comments and where applicable get changes in ASAP to address them.

alex-chew · 2023-03-31T20:31:56Z

This PR was massive.
In addition to moving the entire codegen repo,
it compromised our testing
and refactoring our outstanding PRs to accommodate this refactor is, frankly, an annoying waste of time.

In the future, please attempt to make smaller PRs.

To echo Robin's comment - sorry about the churn. We'll communicate more clearly in the future to try to find better timing in case of big changes like these.

There was no reason, at all, to refactor the Import statements.
But because the import statements were refactored along with all the file movement,
outstanding PRs that actually deliver value must be manually refactored to account for this PR.

I agree that the import-related changes were annoying and I should've caught and reverted them, instead of relying on the IDE refactoring tooling to do the right thing. I'll be careful to isolate file copying from code changes going forward.

Revert "Revert "chore: move codegen logic (#206)" (#208)"

7582e0b

This reverts commit 1080c92.

alex-chew requested a review from a team as a code owner March 30, 2023 03:00

alex-chew requested review from robin-aws and texastony March 30, 2023 03:01

seebees approved these changes Mar 30, 2023

View reviewed changes

alex-chew merged commit a105c76 into main-1.x Mar 30, 2023

alex-chew deleted the revert-208-revert-206-move-codegen-logic-2 branch March 30, 2023 18:14

robin-aws added a commit that referenced this pull request Mar 30, 2023

Also delete the dependency from smithy-dafny-codegen

6e50182

(#209 added it there)

texastony reviewed Mar 31, 2023

View reviewed changes

alex-chew mentioned this pull request Mar 31, 2023

chore(ci): use java17 for codegen, and java8 for tests #221

Merged

texastony restored the revert-208-revert-206-move-codegen-logic-2 branch March 31, 2023 23:02

texastony deleted the revert-208-revert-206-move-codegen-logic-2 branch April 1, 2023 01:05

robin-aws pushed a commit that referenced this pull request Apr 19, 2024

chore: gh_token vs github_token (#209)

76cee6b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert "Revert "chore: move codegen logic (#206)"" #209

Revert "Revert "chore: move codegen logic (#206)"" #209

alex-chew commented Mar 30, 2023

robin-aws commented Mar 30, 2023

seebees left a comment

seebees Mar 30, 2023

alex-chew Mar 30, 2023

texastony Mar 31, 2023 •

edited

Loading

alex-chew Mar 31, 2023

texastony left a comment

texastony Mar 31, 2023

alex-chew Mar 31, 2023

alex-chew Mar 31, 2023

texastony Mar 31, 2023

alex-chew Mar 31, 2023

texastony Mar 31, 2023

alex-chew Mar 31, 2023

texastony Mar 31, 2023

robin-aws Mar 31, 2023

alex-chew Mar 31, 2023

texastony Mar 31, 2023

alex-chew Mar 31, 2023

texastony Mar 31, 2023

alex-chew Mar 31, 2023

robin-aws commented Mar 31, 2023

alex-chew commented Mar 31, 2023

Revert "Revert "chore: move codegen logic (#206)"" #209

Revert "Revert "chore: move codegen logic (#206)"" #209

Conversation

alex-chew commented Mar 30, 2023

robin-aws commented Mar 30, 2023

seebees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

texastony Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

texastony 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

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

robin-aws commented Mar 31, 2023

alex-chew commented Mar 31, 2023

texastony Mar 31, 2023 •

edited

Loading