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

Add graalpy packages to the component directory #8351

Merged
merged 21 commits into from
Dec 4, 2023

Conversation

Akirathan
Copy link
Member

@Akirathan Akirathan commented Nov 21, 2023

Fixes #8294

Pull Request Description

Adds these JAR modules to the component directory inside Engine distribution:

  • graal-language-23.1.0
  • org.bouncycastle.* - these need to be added for graalpy language

Important Notes

  • Remove org.bouncycastle.* packages from runtime.jar fat jar.
  • Make sure that the ./run script preinstalls GraalPy standalone distribution before starting engine tests
    • Note that using python -m venv is only possible from standalone distribution, we cannot distribute graalpython-launcher.
  • Make sure that installation of numpy and its polyglot execution example works.
  • Convert Text to TruffleString before passing to GraalPy - 8ee9a28

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.

@Akirathan Akirathan self-assigned this Nov 21, 2023
Comment on lines +112 to +115
Test.specify "should recognize Text as Python string" <|
py_is_str "Hello" . should_be_true
py_is_str 10 . should_be_false
py_is_str Nothing . should_be_false
Copy link
Member Author

Choose a reason for hiding this comment

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

These tests are failing on develop. I am not sure why the test should correctly marshall strings was passing before.

@Akirathan Akirathan marked this pull request as ready for review November 27, 2023 17:21
@Akirathan
Copy link
Member Author

Akirathan commented Nov 27, 2023

graalpy correctly installs numpy in a virtual environment - https://github.com/enso-org/enso/actions/runs/7008514362/job/19064977553?pr=8351#step:10:4972

GitHub
Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

@Akirathan Akirathan added the CI: No changelog needed Do not require a changelog entry for this PR. label Nov 27, 2023
docs/polyglot/python.md Outdated Show resolved Hide resolved
@Akirathan Akirathan added the CI: Ready to merge This PR is eligible for automatic merge label Nov 29, 2023
Comment on lines +384 to +386
'profiler-tool', licensed under the GNU General Public License, version 2, with the Classpath Exception, is distributed with the engine.
The license file can be found at `licenses/GNU_General_Public_License__version_2__with_the_Classpath_Exception`.
Copyright notices related to this dependency can be found in the directory `org.graalvm.tools.profiler-tool-23.1.0`.
Copy link
Member

Choose a reason for hiding this comment

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

I have no idea if the way we distribute the profiler-tool satisfies the requirements of the GNU GPL license with classpath exception.

If I understand correctly, we are distributing these files inside of our JAR, right? Or is it a separate JAR?

  1. We need to clarify how this dependency is distributed.
  2. Then check if the way we do this is OK with the license - we should consult someone who has more legal knowledge about this.

Copy link
Member Author

Choose a reason for hiding this comment

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

If I understand correctly, we are distributing these files inside of our JAR, right? Or is it a separate JAR?

It is a separate JAR. We basically take those Jars from Maven central and just put it inside the component directory in our distribution, without any modifications.

Then check if the way we do this is OK with the license - we should consult someone who has more legal knowledge about this.

We have always distributed this tool. Just not as a separate Jar, but as part of the GraalVM distribution. It is only now that the legal review tool complains because it can actually discover the license in the Jar. I doubt that there were any major license changes in the community edition of Graal.

Copy link
Member

Choose a reason for hiding this comment

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

I had been in a legal battle about GPL+CP Exception with ASF and I won. That makes me believe I am qualified...

If I understand correctly, we are distributing these files inside of our JAR, right? Or is it a separate JAR?

It is a separate JAR. We basically take those Jars from Maven central
and just put it inside the component directory in our distribution, without any modifications.

Good. Distributing GPL+CPEx JARs is certainly OK and we can even claim the whole thing is under Apache License. Radek is right that repackaging might be tricker - good that we are not doing it.

@mwu-tow mwu-tow removed the CI: Ready to merge This PR is eligible for automatic merge label Nov 29, 2023
@mwu-tow
Copy link
Contributor

mwu-tow commented Nov 29, 2023

It seems that notarization gets broken, removed the "ready to merge" so it can be fixed first.

Copy link
Contributor

@somebody1234 somebody1234 left a comment

Choose a reason for hiding this comment

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

approving the changes to signArchivesMacOs.ts

@Akirathan Akirathan added the CI: Ready to merge This PR is eligible for automatic merge label Dec 4, 2023

fn graalpy_version_from_str(version_string: &str) -> Result<Version> {
let line = version_string.lines().find(|line| line.contains("GraalVM CE")).context(
"There is a Java environment available but it is not recognizable as GraalVM one.",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"There is a Java environment available but it is not recognizable as GraalVM one.",
"There is a Java environment available but it is not recognizable as a GraalVM CE.",

We are checking for Graal CE specifically.

I assume if someone runs GraalVM EE, the current message There is a Java environment available but it is not recognizable as GraalVM one. would be false (GraalVM EE is still GraalVM).

Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Code and config changes look good - approving that.

I'm not sure if the inclusion of profiler-tool in the distribution is allowed by the license, as I noted above - I'm not suggesting it isn't either - I just don't have enough legal experience to be able to assess that.

@mergify mergify bot merged commit a67297a into develop Dec 4, 2023
31 checks passed
@mergify mergify bot deleted the wip/akirathan/8294-install-graalpy branch December 4, 2023 11:51
@Akirathan Akirathan mentioned this pull request Jan 3, 2024
3 tasks
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
Development

Successfully merging this pull request may close these issues.

Install GraalPython
5 participants