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

Remove legacy daml script #20646

Merged
merged 14 commits into from
Jan 28, 2025
Merged

Remove legacy daml script #20646

merged 14 commits into from
Jan 28, 2025

Conversation

samuel-williams-da
Copy link
Contributor

@samuel-williams-da samuel-williams-da commented Jan 22, 2025

Resolves #20639

  • 8bafdb8:

    • Remove daml and scala code for daml script "v1"
    • Renames daml3-script to daml-script
    • Update expandSdkPackages to accept daml3-script and daml-script-lts as simply daml-script, with respective warnings
    • Adds a warning to the runner when attempting to run a legacy daml script (i.e. <=3.2 daml-script), by checking the Module Path of the Script monad (since the package name has been changed)
    • Update release script for daml-libs to only try to include daml-script
    • daml-sdk-head works
    • All tests under compiler/damlc are passing
    • Found a bug, errors from daml3-script submit do not correct refer to the source definition, as such cases matching Script execution failed on commit at <somewhere> have been loosened. To be fixed in a later commit and test changes reverted.
    • Another annoying note, the Submit(.daml) tests use contract keys, so can only run under 2.dev. Should consider splitting them up so we can run a subset of them in 2.1
  • b22abff:

    • Selective replacement of daml3-script with daml-script project wide
    • Turns out the issue with source locations was a missing replacement from above, so that is resolved with this commit
    • Noticed that all the expected files have had whitespace added. Going to revert that in next commit to get changed lines down a bit
  • 3d32a86:

    • Revert all *.EXPECTED.ledger files to their state on master, to reduce changes in this PR
  • 37a616d:

    • Buildifier >:(
    • Tests under daml-script are failing, as still split into daml2-script and daml3-script runners, address in next commit
  • b603bb8:

    • Fix tests under daml-script by removing Daml2ScriptRunnerTest, and dropping the 3 from the various Daml3Script tests
    • Also needed to update an error assertion as daml3-script uses engine.free, over its own free implementation
  • f135c2c:

    • LSP cancellation test was failing, as once moved over to new script implementation, cancellation on long running pure computations wasn't working, and was delaying the cancellation request until after the computation finished. This was fixed by updating Remy's Free machine to calculate the initial Free monad (before script interpretation) in a Future, and having it check for cancellation there as well.
  • 263dc4e:

    • Update Submit.daml test to use ifdef so it can run in 2.1, This required adding if= to diagnostic expectations so they can be gated by defs
  • ef1de7a:

    • Update hoogle and haddock templates for daml script
  • 58819c3:

    • Move the legacy script check to the runner, so it can use the same error types
    • Add test for this
  • d04c037:

    • Test was uploading dars every time, and previous test uploaded the real daml-script. Naturally the upload of the fake daml-script after that fails, as its not a valid upgrade. Updated the test to not upload the fake daml-script, as it's not needed.

TODO:

  • Ensure documentation logic works
    • Hard to know until docs consumes it
  • Account for canton issues
    • Sounds like a Monday Sam problem

@samuel-williams-da samuel-williams-da marked this pull request as ready for review January 24, 2025 16:13
@samuel-williams-da samuel-williams-da requested review from a team as code owners January 24, 2025 16:13
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can use an ifdef here and keep it running in 2.1


The Daml Script library defines the API used to implement Daml scripts. See :doc:`/daml-script/index`:: for more information on Daml script.
NOTE: This is the unstable Daml 3 script library replacement. It can and will make breaking changes without warning up to the release of Daml 3.0.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Outdated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same with line below

Copy link
Contributor

@dylant-da dylant-da left a comment

Choose a reason for hiding this comment

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

Reviewed on a call - LGTM modulo comments

Copy link
Contributor

@dylant-da dylant-da left a comment

Choose a reason for hiding this comment

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

LGTM

@samuel-williams-da samuel-williams-da merged commit 0e46e9d into main Jan 28, 2025
14 checks passed
@samuel-williams-da samuel-williams-da deleted the sw/remove-old-daml-script branch January 28, 2025 16:43
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.

Remove legacy daml script
2 participants