Python: Get most CI green #530
Conversation
| uses: gradle/gradle-build-action@v2 | ||
| with: | ||
| arguments: :codegen-client:pTML :codegen-core:pTML :rust-runtime:pTML | ||
| build-root-directory: smithy-dafny-codegen-modules/smithy-rs | ||
|
|
||
| - name: Install smithy-dafny-codegen Python dependencies locally |
There was a problem hiding this comment.
Just FYI I had to do this because smithy-rs packages aren't published at all yet, but I assume you have to do it here because you're still patching the smithy-python code?
There was a problem hiding this comment.
Also it might be worth a reusable .github/actions for this since it's not very DRY.
There was a problem hiding this comment.
Yes and yes (both because Smithy-Python isn't on Maven and because I have patches.)
There was a problem hiding this comment.
Made a GHA for "Install Smithy-Dafny codegen dependencies"
There was a problem hiding this comment.
FYI I snuck these targets in for human usage too: https://github.com/smithy-lang/smithy-dafny/blob/main-1.x/Makefile#L13 :)
Probably good to add the python dependency there too, perhaps in this PR so we don't forget?
| @@ -5,9 +5,6 @@ rootProject.name = "smithy-dafny" | |||
| include(":smithy-dafny-codegen") | |||
| include(":smithy-dafny-codegen-cli") | |||
| include(":smithy-dafny-codegen-test") | |||
| // TODO: Once Smithy-Python is published to Maven, and we do not rely on a fork, use that | |||
There was a problem hiding this comment.
This should probably be tracked in an issue for both Python and Rust
There was a problem hiding this comment.
You mean an issue in Smithy-Python/rs that says "Publish codegen to Maven"? I agree
There was a problem hiding this comment.
Python: smithy-lang/smithy-python#296
Rust already had one: smithy-lang/smithy-rs#421
| DISABLED_TESTS.add("SimpleTypes/SimpleFloat"); | ||
| DISABLED_TESTS.add("SimpleTypes/SimpleShort"); | ||
| DISABLED_TESTS.add("SimpleTypes/SimpleTimestamp"); | ||
| DISABLED_TESTS.add("aws-sdks/ddb-lite"); |
There was a problem hiding this comment.
FYI the -lite models should be very easy to turn on as well, since the projection is handled before hitting any language-specific logic.
There was a problem hiding this comment.
Took a look, I'll leave these unimplemented for now.
Even though the models are "lite", they use the gradle build process, which seems to need extra work to get working.
I think Python and Go will need extra work to figure out how to pass in extra args for the "dependency-module-names" and "library-name" flags.
There was a problem hiding this comment.
Fair enough, there's no real motivation to get those working now anyway - they are mainly useful during the development of new languages and you're already past that stage.
| _transpile_implementation_all: transpile_implementation | ||
|
|
||
| # TODO-new-cli: As part of supporting Dafny 4.8+, Java and NET SHOULD migrate to the new Dafny CLI, |
There was a problem hiding this comment.
This feels slightly off and could be more precise - is it because the legacy CLI will eventually be fully deprecated? It seems more like other languages need features that are only in the new CLI.
There was a problem hiding this comment.
Yes, but the more immediate desire is that Java/NET/Python/Go have the same build command.
This reduces risk of accidental "forks" in one build process or the other.
(I know we will need some Makefile checking that says "only add --compile-suffix:1 for Java/NET", but that is a small "fork".)
This is meant to assert that "we have a 4.8 upgrade coming up, these targets SHOULD be merged as part of that upgrade"
There was a problem hiding this comment.
The _new_cli duplication seems reasonable to me. Only question is whether this PR also ensures they are as well tested. It seems like it is since Python will only use them?
There was a problem hiding this comment.
Yes, right now only Python.
My goal is to get this into main soon so Rishav and Shubham can onboard to a "shared" solution.
| @@ -2,7 +2,7 @@ | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
| include "SimpleAggregateImpl.dfy" | |||
|
|
|||
| module SimpleAggregate refines AbstractSimpleAggregateService { | |||
| module {:extern "simple.aggregate.internaldafny"} SimpleAggregate refines AbstractSimpleAggregateService { | |||
There was a problem hiding this comment.
Is this a breaking change for existing projects?
There was a problem hiding this comment.
Nope! This is the current state of main-1.x.
I'm putting the extern names back in the python-reviewed branch so that when I merge this branch in, there is no diff between its Dafny source and main's.
There was a problem hiding this comment.
Ah I actually missed that this is not targeting main-1.x itself.
There was a problem hiding this comment.
Consider implementing --generate project-files for Python in the future, so many libraries don't have to provide this file.
There was a problem hiding this comment.
Good to know, thanks. I didn't realize we had this
Corrected GetStringUTF8 to use this trait Corrected the description of the existing test that incorrectly said the string was in UTF-8 (it's a Dafny string so it's implicitly UTF-16 instead) Added identically structured test of GetStringUTF8 Updated benerated Rust code to make sure this case is handled too.
Issue #, if available:
Description of changes:
This gets most CI green. (Caveat: it does this by having separate transpile commands for NET/Java vs Python, but this SHOULD only be separate until NET/Java get on 4.8.)
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.