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

Scala object serializer supports copying. #16

Conversation

sageserpent-open
Copy link
Contributor

This follows on from altoo-ag/akka-kryo-serialization#297.

I've broken up the PR into three commits.

  1. Add a test for the existing serialization / deserialization functionality provided by ScalaObjectSerializer.
  2. Add a failing test to reproduce the problem with copying.
  3. Make the failing test pass.

@sageserpent-open
Copy link
Contributor Author

Oh drat - I had to push to a fork of this repository, so could someone approve the failing CI workflow run, please?

@sageserpent-open
Copy link
Contributor Author

Thanks @danischroeter for triggering the workflow run.

Unfortunately it failed when building for Scala 2.12; I've pushed a new commit (and this time, I ran the full cross build locally, lesson learned).

Would you trigger the CI workflow again when you have a moment?

@sageserpent-open
Copy link
Contributor Author

sageserpent-open commented Sep 26, 2024

@danischroeter This is odd - the CI build is still failing, but with this error:

Run sbt +test
/home/runner/work/_temp/b0abd[6](https://github.com/altoo-ag/scala-kryo-serialization/actions/runs/11047497892/job/30690517455#step:4:7)a0-fd9d-426f-b3c2-0542eb49ae20.sh: line 1: sbt: command not found
Error: Process completed with exit code 12

7.

I've not changed any of the workflow configuration in this PR, so I assume this is a transient glitch - perhaps due to the node switch from node12 to node16?

Other than asking you to re-trigger again, I'm not sure what to do. If this failure is repeatable, then perhaps running the CI action on branch main might help diagnose what's wrong?

@danischroeter
Copy link
Member

Jep I already created another issue... #17

@sageserpent-open
Copy link
Contributor Author

I tried re-triggering the CI build on the Americium repository, this uses a later version of setup-java step.

It's working nicely right now, finding SBT.

An excerpt:

jobs:
  build:

    runs-on: ubuntu-latest

    steps:
      - uses: actions/checkout@v4
      - name: Set up JDK 17
        uses: actions/setup-java@v4
        with:
          java-version: '17'
          distribution: 'temurin'
          cache: 'sbt'
      - name: Run tests
        run: sbt +test

@sageserpent-open
Copy link
Contributor Author

@danischroeter I took the liberty of upgrading the GitHub workflow steps to v4 for checkout and Java setup.

I don't know whether you folks have strong feelings about sticking with v2, but if you're happy with the upgrade being bundled in with this PR, please re-trigger the CI build.

If not, let me know and I'll git reset --hard on the PR branch and force-push to remove commit f7887cc.

@sageserpent-open sageserpent-open force-pushed the scala-object-serializer-supports-is-immutable branch from f7887cc to 86ddf36 Compare September 26, 2024 09:59
@sageserpent-open
Copy link
Contributor Author

Drat - I only cutover the fullTest.yml workflow - I've force-pushed to include the pullCI.yml as well.

Another re-trigger, please... 😅

@danischroeter
Copy link
Member

@sageserpent-open thx for the effort!
still did not work - will try to fix this in a separate pr and then resume here...
just need to get a few other things done first.

@sageserpent-open
Copy link
Contributor Author

@danischroeter no problem, sorry it didn’t work. I’ll remove that last commit as it’s just noise on this PR and wait for you folks to come back as and when you’re ready.

@danischroeter
Copy link
Member

@sageserpent-open
#17 is fixed :) - ubuntu-latest was the problem
can you rebase pls?

@sageserpent-open sageserpent-open force-pushed the scala-object-serializer-supports-is-immutable branch from 86ddf36 to 98873f8 Compare September 26, 2024 13:40
@sageserpent-open
Copy link
Contributor Author

@danischroeter Excellent, thanks.

I've rebased and force-pushed. Would you trigger the build again, please...

Copy link
Member

@danischroeter danischroeter left a comment

Choose a reason for hiding this comment

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

Nice thx - just one small testing improvement.

…jectSerializerTest` - the tests themselves now assert on *identity*, but the helpers are used to assert on *state*.
@danischroeter danischroeter merged commit 490b799 into altoo-ag:main Sep 26, 2024
1 check passed
@danischroeter
Copy link
Member

@sageserpent-open thx for your contribution! We planned to do a release of scala-kryo-serialization in ~ 2weeks when doing upgrades also for pekko 1.1. Or if it's very crucial for you we can do a release for your change...

@sageserpent-open
Copy link
Contributor Author

sageserpent-open commented Sep 26, 2024

@danischroeter, @nvollmar , it was a pleasure working with you folks. I’m in no hurry - got a workaround in place, just wanted to contribute the fix upstream. I’ll keep an eye out for your next release.

Cheers for now…

@sageserpent-open sageserpent-open deleted the scala-object-serializer-supports-is-immutable branch September 26, 2024 17:12
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