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

Allow to set tonel writer version to use for export #1798

Merged
merged 2 commits into from
Jan 31, 2024

Conversation

jecisc
Copy link
Contributor

@jecisc jecisc commented Jan 31, 2024

This change allows one to set the tonel writer version to use for a project in the .properties file of a project.

For project available on P11 and P12, to avoid to rewrite all the file each time we commit on a different version of Pharo we can provide those properties:

{
	#format : #tonel,
	#version: #'1.0'
}

And the v1 format will be used on both versions of Pharo.

If it does not find the version provided, it will fallback to the default version of the image.

This change allows to set the tonel version to use in the .properties file of the repository
@jecisc
Copy link
Contributor Author

jecisc commented Jan 31, 2024

Failing tests are not related and seems to come from the current state of Iceberg

@tinchodias
Copy link
Collaborator

This feature would be useful for Bloc but I think for all projects that work in Pharo 11 and 12. We manually change the default version in TonelWriter class but from time to time we forget in a new image and change unintentionally the tonel version in the changed package.

@jecisc
Copy link
Contributor Author

jecisc commented Jan 31, 2024

Yes! The goal here is to define it once in the properties and then nobody has to care anymore :)

@tesonep tesonep merged commit b25aa14 into pharo-vcs:dev-2.0 Jan 31, 2024
0 of 3 checks passed
@dalehenrich
Copy link

dalehenrich commented Jan 31, 2024

Cyril, just want to mention that there are other users of the tonel format and while it is fun to add new features "as needed" you should consider the impact your changes will have on shared projects ...

There used to be an actual tonel spec that was used for this purpose and the process of maintaining and conforming to the project should allow for moving forward in an orderly fashion ...

I actually am in favor of including version information in "a properties file" but I think the devil is in the details ... for example because of the way that Pharo used to treat symbols and strings as interchangeable (and thanks for your effort in this area!), I was forced change the Rowan tonel implementation to read/write the symbols back out the way they came in an to attempt to preserve the integrity of the original repository ...

So far Pharo and GemStone/Rowan has been able to steer clear of each other in terms of the exact details of the disk representation details for tonel ... but once GemStone/Rowan is released, tonel will be in active use by both GemStone and Pharo in a number of SHARED github repositories and it is imperative that we are tolerant of each others foibles ....

I know that there is a principle in the original tonel spec that all readers/writers preserve class/package/method/repository attributes so that platforms can actually share the code, without sharing feature sets ...

I actually have several RowanSample* repositories where I have created branches with particular disk-based content that I can use in my unit tests ... I created this particular branch a couple of years ago so that I could verify that I could "faithfully preserve" Pharo's use of symbols as keys and I regularly run unit tests using that branch to "maintain conformance to non-standard tonel formats" ... while I haven't looked closely at the Rowan code, it does look like I was able to do that without any special flags on disk ...

I "taught" Rowan to detect and preserve the cases when Symbols were used where Strings were supposed to be used ...

And it seems to me that from a practical perspective, it makes sense to create a shared git repository with a series of branches each containing examples of the unique features of each of the platforms so that unit tests can be written by each platform to confirm that they can faithfully read and preserve on write the disk formats used by the other platforms ....

Proposals for changes would be added as examples and when all of the platforms can successfully read/write the new format, it can be added the mix ... We should be able to create github actions to automatically run the unit tests, so the validation can be automated ... and changes that fail the unit tests would then be the only time it is necessary to talk about changes that would need to require changes to the implementations ...

@jecisc
Copy link
Contributor Author

jecisc commented Jan 31, 2024

Hi Dale,

From what I know this property file I updated is not related to Tonel but to Iceberg that is used by Pharo to store some properties. Maybe I'm wrong but I think this file is ignored in other smalltalk flavors?

So I'm not sure this change anything for Gemstone. But maybe I'm wrong?

@jecisc jecisc deleted the set-tonel-version branch January 31, 2024 17:39
@dalehenrich
Copy link

@jecisc, well that file looks an awful lot like this properties.st file ... we may already be colliding ... with my addition of a #convention field :) ... I know that there are number of these little files laying around on disk and if we have a repo with the current/active layouts on display, we can make informed decisions about what needs to be preserved and what field names we are using ...

As I think about this file, I think it is currently readonly, so it will be safe to add fields manually until someone decides that they want to write it without preserving fields, then we would be in trouble .....

@jecisc
Copy link
Contributor Author

jecisc commented Jan 31, 2024

Oh I see that you also have it.

In Pharo we are preserving everything that we read in it and we ignore every field that does not interest us.

The problem might be that symbol/strings export but my change does not change anything about this. I think pharo exports symbol/symbol for now which is not good for gemstone :/ This would need to be updated

@dalehenrich
Copy link

... and to be more friendly, the unrecognized fields need to be retained IFF the file is ever written ... presumably some process creates/writes that file and that process should be prepared to read/write if updates are needed ...

I really think we need to have a shared repository Sample where we can place a Pharo repository on one branch and a Gemstone repository on another branch and we should ensure that we can properly read/update each sample ... and if there are multiple formats of data, then we need a branch per version ...

We should probably do this for filetree format repositories as well, since they are still used in the wild ...

Having a set of standard format examples is really the only way for us to know that we won't stomp on each other when we read/write ...

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.

4 participants