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

Revert "Add Meta.engine_version" #11336

Merged
merged 1 commit into from
Oct 16, 2024

Conversation

radeusgd
Copy link
Member

Reverts #11320

@JaroslavTulach suggested that such a text-based API is an anti-pattern and so we should back out the PR.

When running locally, we probably should be able to rely on digging through logs to find the engine version that was picked up.

@radeusgd radeusgd added the CI: No changelog needed Do not require a changelog entry for this PR. label Oct 16, 2024
@jdunkerley
Copy link
Member

Do we not want a way to get the version of the engine and the libs at runtime?

@radeusgd
Copy link
Member Author

Do we not want a way to get the version of the engine and the libs at runtime?

If it is supposed to be used by code, then indeed it needs more design. This was just a little debug tool. I think it could have been useful as a debug tool, but I don't want to argue about it.

I guess ideally, we'd want the IDE to display the currently running engine version next to the GUI version. This could probably be achieved by adding some Language Server endpoint. But that requires additional coordination between GUI and LS teams.

@somebody1234
Copy link
Contributor

i guess @4e6 @MrFlashAccount for discussion
personally i don't think there's any real preference for the exact API, as long as it's callable and returns info dashboard will be able to use it.
so if anything the discussion would be on what info we want - (just the engine version? git branch/hash so that we can debug engine binaries taken from PRs?)

@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Oct 16, 2024
@mergify mergify bot merged commit 069e236 into develop Oct 16, 2024
42 checks passed
@mergify mergify bot deleted the revert-11320-wip/radeusgd/engine-version branch October 16, 2024 12:09
@jdunkerley
Copy link
Member

Do we not want a way to get the version of the engine and the libs at runtime?

If it is supposed to be used by code, then indeed it needs more design. This was just a little debug tool. I think it could have been useful as a debug tool, but I don't want to argue about it.

We should be able to get a version within the libs code but yes should be built properly.
GUI should ideally show version details for running project somewhere so we can get it when helping someone.

@JaroslavTulach
Copy link
Member

Do we not want a way to get the version of the engine and the libs at runtime?

I believe we should expose the version to the code. And we should expose not only a single version, but multiple ones! To quote Practical API Design Chapter 2: "The Motivation to Create an API", page 21:

Each piece of a modular application has a version number, usually a set of natural numbers separated by dots, such as 1.34.8. When a new version is released, it should have a new and larger version number .... You can express dependencies on other components of the modular system by specifying the identification name of such a component and the minimal needed version.

Some versions of components can contain bugs that need workarounds. For this reason, a
secondary version—an implementation version—is usually associated with a component. In
contrast to the specification version, this is usually a string, such as Build20050611, and can
only be tested for equality.

E.g. it is OK to have Runtime.version type=..Engine and return "2024.4.2" or Runtime.version type=..Commit and return "1a0ae7c84d8d79c9b6d825c34557a5cd2ac97954".

However I am against exposing concatenated text messages that require parsing to be processed by a machine. It 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 information 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).

Last, but not least I agreed to disagree with @radeusgd that exposing an API annotated with ## PRIVATE creates no obligations. It does! I can't force that policy to the whole libraries team, but I am ready to apply such policy when it comes to the Enso engine API (e.g. builtins).

Next time anyway gets an idea to modify a builtin, please involve @JaroslavTulach in the review.

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.

5 participants