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

Serialize UUID for non-library modules #9057

Merged
merged 3 commits into from
Feb 15, 2024

Conversation

hubertp
Copy link
Contributor

@hubertp hubertp commented Feb 14, 2024

Pull Request Description

Missing ID's in IR meant that instrumentation wouldn't be applied for loaded modules. This is the reason why after a restart engine wouldn't send any expression updates.

Closes #8689.

Important notes

After the change
Kazam_screencast_00038.webm

The video somehow doesn't show that all nodes are loaded after the restart, but once I moved the screen they are there. This appears to be a bug in the recording somehow.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.
    • Change caches version ID

Missing ID's in IR meant that instrumentation wouldn't be applied for
loaded modules. This is the reason why after a restart engine wouldn't
send **any** expression updates.
@hubertp hubertp added the CI: No changelog needed Do not require a changelog entry for this PR. label Feb 14, 2024
@hubertp
Copy link
Contributor Author

hubertp commented Feb 14, 2024

Should also fix #8941

@JaroslavTulach
Copy link
Member

Should also fix #8941

Yes, the problem is going to be fixed, but only after clearing local caches. That's not fully ideal.

Please change caches version ID as the serialized form for .ir files has changed and previous caches shall not be used.

@hubertp hubertp added the CI: Ready to merge This PR is eligible for automatic merge label Feb 15, 2024
@hubertp
Copy link
Contributor Author

hubertp commented Feb 15, 2024

I gave up on adding a unit test. It looks like UUIDs aren't added to IdentifierLocation by default in tests which makes it particularly hard to set it up without changing the parser. This change is really annoying and I already wasted too much time on it.

@mergify mergify bot merged commit d29c2cd into develop Feb 15, 2024
27 of 29 checks passed
@mergify mergify bot deleted the wip/hubert/8689-restart-language-server branch February 15, 2024 16:50
@@ -23,7 +23,7 @@ import scala.annotation.unused
*/

@SerialVersionUID(
8160L // Use BindingsMap
9057L // Use BindingsMap
Copy link
Member

Choose a reason for hiding this comment

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

We are no longer using serialVersionUID as we have our own Persistance framework. Instructions to version caches are here.

Sorry for not stating that more clearly when requesting you to "Change caches version ID", but the last change to the documentation was done by you, so I expected you to remember the context.

As a result of not versioning the caches properly, I have to clean rm -rf ~/.local/shared/enso/caches/ir to fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
2 participants