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

Fix observability support and bump connector version #5

Merged
merged 3 commits into from
Mar 26, 2024

Conversation

NipunaRanasinghe
Copy link
Contributor

Purpose

$subject.

Related to ballerina-platform/ballerina-library#6185

Examples

Checklist

  • Linked to an issue
  • Updated the changelog
  • Added tests
  • Updated the spec
  • Checked native-image compatibility

observabilityIncluded = false

[platform.java17]
graalvmCompatible = true
Copy link
Member

Choose a reason for hiding this comment

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

Why have we set graalvmCompatible flag to false ?

Copy link
Contributor Author

@NipunaRanasinghe NipunaRanasinghe Mar 26, 2024

Choose a reason for hiding this comment

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

@ayeshLK Just to clarify, I haven't set the graalVM compatibility to false in here.

here I've simply removed the graalvm property, because all pure ballerina connectors (which does not require any native dependencies) should get detected as graalVM compatible by default.

Copy link
Member

Choose a reason for hiding this comment

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

AFAIR we have marked other connectors with this flag. @ThisaruGuruge any idea ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ayeshLK Just to clarify, I haven't set the graalVM compatibility to false in here.

here I've simply removed the graalvm property, because all pure ballerina connectors (which does not require any native dependencies) should get detected as graalVM compatible by default.

@TharmiganK appreciate if you can verify

Copy link
Member

Choose a reason for hiding this comment

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

AFAIR we have marked other connectors with this flag. @ThisaruGuruge any idea ?

If the platform is any (i.e. there are no Java dependencies), the GraalVM compatibility should be inferred from the dependency graph. So in this case, I don't think we need this. We don't have to provide anything under the platform.java17 field in the toml, if the package is platform.any, as I understand it.

Choose a reason for hiding this comment

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

If this flag is not set then the GraalVM compatiblity is inferred from the package and its dependencies. So if the package is a pure ballerina package then, it is GraalVM compatible if and only if the dependencies are GraalVM compatible. If the dependencies are from ballerina platform, then all of them are are GraalVM compatible. In that case this flag is not required.

The standard approach is to mark the ballerina package as GraalVM compatible true/false only when we have java dependencies/native code. If you have pure ballerina package skip this flag so that the bal tool and central will infer it from the dependencies.

@NipunaRanasinghe NipunaRanasinghe merged commit 43a05b6 into ballerina-platform:main Mar 26, 2024
2 checks passed
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.

5 participants