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

Change layout of local library search path in order to be able to move Round_Spec.enso back to Tests #7634

Merged
merged 30 commits into from
Sep 1, 2023

Conversation

radeusgd
Copy link
Member

@radeusgd radeusgd commented Aug 22, 2023

Pull Request Description

  • Closes Improve experience of inter-project imports in local projects #7633
  • Moves Round_Spec.enso from published Standard.Test into our test/Tests project; the Table_Tests that depend on it, simply import enso_dev.Tests.
  • Changes the layout of the local libraries directory:
    • It used to be root/<namespace>/<name>.
    • Now it is root/<dir> - the namespace and name are now read from package.yaml instead.
  • Adds the parent directory of the current project to the default ENSO_LIBRARY_PATH.
    • It is treated as a secondary path, so the default ENSO_HOME/lib still takes precedence.
    • This allows projects to reference and load 'sibling' projects easily - the only requirement is for the project to enable prefer-local-libraries: true or add the other local project to its edition. The edition resolution logic is not changed.

Important Notes

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.

@radeusgd radeusgd linked an issue Aug 22, 2023 that may be closed by this pull request
7 tasks
@radeusgd radeusgd force-pushed the wip/radeusgd/test-suite-dependencies branch from cfa30ad to e6dc959 Compare August 24, 2023 11:57
@radeusgd
Copy link
Member Author

As of commit e6dc959 I was able to run:

> $env:NO_LOG_MASKING=1
> enso run .\test\Table_Tests\src\Common_Table_Operations\Column_Operations_Spec.enso
...
[info] [2023-08-24T11:56:08.23Z] [enso.org.enso.interpreter.runtime.DefaultPackageRepository] Found library Standard.Test @ 0.0.0-dev at [C:\Users\progr\AppData\Local\enso\dist\0.0.0-dev\lib\Standard\Test\0.0.0-dev].
[info] [2023-08-24T11:56:08.241Z] [enso.org.enso.interpreter.runtime.DefaultPackageRepository] Found library enso_dev.Tests @ local at [C:\NBO\enso\test\Tests].
...
106 tests succeeded.
0 tests failed.
5 tests skipped.

Now to change the default search paths to make the env var override unnecessary.

@radeusgd radeusgd force-pushed the wip/radeusgd/test-suite-dependencies branch from 3d4704c to 66dac58 Compare August 28, 2023 14:18
@radeusgd radeusgd changed the title Move Round_Spec.enso back to Tests, try linking Tests from Table_Tests Change layout of local library search path in order to be able to move Round_Spec.enso back to Tests Aug 28, 2023
@radeusgd radeusgd force-pushed the wip/radeusgd/test-suite-dependencies branch from 66dac58 to b5bf679 Compare August 28, 2023 15:53
@radeusgd radeusgd self-assigned this Aug 28, 2023
@radeusgd radeusgd marked this pull request as ready for review August 28, 2023 15:56
}

"LocalLibraryManager" should {
"find the libraries it has itself created" in {
Copy link
Member Author

Choose a reason for hiding this comment

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

This test is partially redundant with

"create a library project and include it on the list of local projects" in {
but it checks some more cases and is more a 'unit test' whereas the other is more an 'integration test'.

So IMO it may make sense to keep both - the role of the newly added one is to ensure the logic for creating a library is kept in-sync with the logic for resolving them. The other test is just an integration test checking library management.

@@ -17,6 +16,8 @@ from Standard.Database.Errors import all
from Standard.Test import Test, Problems
import Standard.Test.Extensions

import enso_dev.Tests.Data.Round_Spec
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm really glad we could get this working 😀

I think this also opens up doors to more logic sharing where we can import parts of tests from Tests in other test suites like Table_Tests.

license: MIT
author: enso-dev@enso.org
maintainer: enso-dev@enso.org
prefer-local-libraries: true
Copy link
Member Author

Choose a reason for hiding this comment

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

Another, IMO better, way to do this would be to declare the depended-on library in the edition override:

edition:
   extends: 2023.123-dev
   libraries:
     - name: enso_dev.Tests
       repository: local

However to do so, we need to specify some edition (like 2023.123-dev example), but we don't really have one to specify. I guess we should extend the edition resolution to be able to specify extends: default. Currently that is not possible, but it should be a relatively simple extension.

@jdunkerley @JaroslavTulach shall I create a ticket to allow extends: default? Seems +- aligned with the direction #7639 is going in.

Copy link
Member

Choose a reason for hiding this comment

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

shall I create a ticket .... aligned with the direction https://github.com/orgs/enso-org/discussions/7639 is going in.

#7639 discusses incompatibilities between engine & IDE. I am unsure how that relates and affects editions. If an existing concept like prefer-local-libraries works, it is easier to suggest to use it than think about consequences of changing extends mechanism.

CHANGELOG.md Outdated
@@ -923,6 +923,8 @@
- [Only use types as State keys][7585]
- [Allow Java Enums in case of branches][7607]
- [Notification about the project rename action][7613]
- [Changed layout of local libraries directory, making it easier to
reference projects next to each other][7634]
Copy link
Member

Choose a reason for hiding this comment

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

Supporting depedencies among sibling projects

Comment on lines 89 to 93
if (Files.exists(libraryPath)) {
// TODO [RW] we could try finding alternative names (as directory name does not matter for local libraries), to find a free name
// This can be done as part of #1877
throw new RuntimeException("Local library already exists")
}
Copy link
Member Author

Choose a reason for hiding this comment

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

The issue #1877 was closed some time ago as part of a cleanup, but it indeed remains unsolved - there are a few TODOs referencing it and indeed the error handling in that part is not fully developed but a shortcut.

It may be worth to re-open the issue. However, this part of the API is not yet being used by the IDE. I guess we may wait until it starts being used to re-visit these improvements. But let's keep in mind there is some missing functionality there that should be filled-in once this is actually used.

@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Aug 31, 2023
@mergify mergify bot merged commit 87ce786 into develop Sep 1, 2023
24 checks passed
@mergify mergify bot deleted the wip/radeusgd/test-suite-dependencies branch September 1, 2023 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve experience of inter-project imports in local projects
4 participants