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.

Sign up for GitHub

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 Meta.engine_version #11320

Merged
merged 3 commits into from
Oct 15, 2024
Merged

Add Meta.engine_version #11320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4c5d4c5
Add `Meta.engine_version`
radeusgd Oct 14, 2024
01e3aff
reorder, change import
radeusgd Oct 14, 2024
cbf12d8
truffle boundary
radeusgd Oct 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions distribution/lib/Standard/Base/0.0.0-dev/src/Meta.enso
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -688,3 +688,9 @@ type Instrumentor
- fqn: fully qualified name.
find_type_by_qualified_name : Text -> Any
find_type_by_qualified_name fqn = @Builtin_Method "Meta.find_type_by_qualified_name"

## PRIVATE
ADVANCED
Returns the version of the currently running Enso engine.
engine_version : Text
Copy link
Member

Choose a reason for hiding this comment

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

Please move the @Builltin_Method into a private module to be ready for to get ready for

delegate the builtin method here, if you want to expose it to users (even just with ## PRIVATE comment).

engine_version = @Builtin_Method "Meta.engine_version"
45 changes: 45 additions & 0 deletions ...main/java/org/enso/interpreter/node/expression/builtin/meta/CurrentEngineVersionNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package org.enso.interpreter.node.expression.builtin.meta;

import com.oracle.truffle.api.CompilerDirectives;
import com.oracle.truffle.api.nodes.Node;
import org.enso.interpreter.dsl.BuiltinMethod;
import org.enso.interpreter.runtime.data.text.Text;
import org.enso.version.BuildVersion;

@BuiltinMethod(
type = "Meta",
name = "engine_version",
description = "Returns the version of the currently running Enso engine.",
autoRegister = false)
public class CurrentEngineVersionNode extends Node {

public Text execute() {
return getCurrentVersion();
}

@CompilerDirectives.TruffleBoundary
private Text getCurrentVersion() {
StringBuilder sb = new StringBuilder();
Copy link
Member

Choose a reason for hiding this comment

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

This is not really an engine_version. Engine version is 0.0.0-dev - this is some string that needs to be parsed to get the engine version.

Copy link
Member

Choose a reason for hiding this comment

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

Exposing text messages as an API is a well known antipattern which has bitten many. See for example Practical API Design, Chapter 3: "Determining what makes a good API", page 31:

Beware of situations where there is no alternative to parsing text messages! If the informa-
tion isn’t available in other ways, people will parse any textual output generated by your code.
For example, this happened to the JDK’s Exception.printStackTrace(OutputStream).

Copy link
Member Author

Choose a reason for hiding this comment

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

Will you be ok if I rename this method to engine_version_info or something else?

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 whole point of this method is to have a single method to get a quick glance of the currently running version.

Just 0.0.0-dev version string is not enough to get that - it will be the same string for every dev build, regardless of which commit it was build from, so that tells very little.

If I split this up into separate methods, it will be tedious to reconstruct - the whole rationale is to easily get a debug overview.

This method is not meant to be used or parsed by users, if anyone uses it like that - I guess the usage is the anti-pattern. Is the ability for users to abuse a helper debug method enough reason not to have it?

Copy link
Member

Choose a reason for hiding this comment

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

Is the ability for users to abuse a helper debug method enough reason not to have it?

Yes, it is! By exposing a text API like this either of:

  • changing the text message
  • removing the method

constitutes an incompatible change and it is a potential threat to existing Enso users.

In addition to that. In its current form this change doesn't meet requirements for an API change. There is a checkbox:

[ ] Unit tests have been written where possible.

Where else shall there be tests then when doing an API change?

sb.append("Enso Engine Version: ");
sb.append(BuildVersion.ensoVersion());
sb.append("\nDefault Edition: ");
Copy link
Member

Choose a reason for hiding this comment

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

I am surprised that on your Windows this displays newlines? Shouldn't you rather use System.lineSeparator()?

Copy link
Member Author

Choose a reason for hiding this comment

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

It works fine with \n and we use it all over the place, e.g. in displaying the Test results. So I'd prefer to keep it as is.

sb.append(BuildVersion.currentEdition());

sb.append("\nCompiled with GraalVM ");
sb.append(BuildVersion.graalVersion());
sb.append(", Scalac ");
sb.append(BuildVersion.scalacVersion());

sb.append("\nBased on commit ");
sb.append(BuildVersion.commit());
sb.append(" (at ");
sb.append(BuildVersion.latestCommitDate());
sb.append(")\non ref ");
sb.append(BuildVersion.ref());
if (BuildVersion.isDirty()) {
sb.append("\n(with uncommitted changes)");
}

return Text.create(sb.toString());
}
}
5 changes: 5 additions & 0 deletions test/Base_Tests/src/Semantic/Meta_Spec.enso
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,6 +476,11 @@ add_specs suite_builder =
typ = Meta.Type.find fqn
typ . should_equal meta_type

suite_builder.group "Engine Metadata" group_builder->
group_builder.specify "should return engine version" <|
version = Meta.engine_version
version.should_contain "Enso Engine Version:"

main filter=Nothing =
suite = Test.build suite_builder->
add_specs suite_builder
Expand Down
Loading