ProtonMail / proton-bridge

Proton Mail Bridge application
GNU General Public License v3.0
1.16k stars 157 forks source link

Strange versioning - Project merge suggestion #100

Closed Spixmaster closed 3 years ago

Spixmaster commented 4 years ago

Why did this project switch from v1.3.3 to ie-x.x.x? I assume that "ie" stands for import, export but as this is only a new feature would not just a new version major make more sense?

molaeiali commented 3 years ago

Also, which tag is 1.4.4? It's really confusing for me as an Arch Linux AUR package maintainer @jameshoulahan @horejsek @cuthix @skooda

jameshoulahan commented 3 years ago

From now on, all import-export releases will be tagged as ie-MAJOR.MINOR.PATCH and all bridge releases will be tagged as br-MAJOR.MINOR.PATCH.

1.4.4 will be merged to github shortly and will be tagged br-1.4.4

Spixmaster commented 3 years ago

@jameshoulahan Why do not you just added the import-export tool as a new version major into ProtonMail Bridge? This is really confusing. Having two versioning systems for on software is odd. I do not even know whether those two features (ProtonMail Bridge and import-export tool) are still one software. Please, change to one consistent versioning and tag commits when a new release is available.

jameshoulahan commented 3 years ago

They are two distinct applications:

Initially, the repositories and codebases for the apps were separate. In order to reduce the maintenance burden, we rewrote the Import-Export app to share a lot of the existing (and more polished) Bridge codebase. This repository is thus now a "monorepo", if you will, and as it produces two apps, it requires two separate versioning systems.

Spixmaster commented 3 years ago

@jameshoulahan With the background information it can be understood. Nevertheless, I would recommend including your description in the readme file. There is no statement in the readme from which users can know this.

jameshoulahan commented 3 years ago

The readme's title already indicates that both apps are present. It also contains two top level sections, one for bridge and one for import-export.

But I agree the first sentence is very misleading:

This repository holds the ProtonMail Bridge application.

should read something like:

This repository acts as a monorepo for the ProtonMail Bridge and Import-Export applications.

We'll also add a section explaining how the tags work, as that seems to have caused some confusion as well.

Thanks for the valuable feedback!

kortschak commented 3 years ago

This approach to versioning breaks go modules' understanding of the what is going on, there is one go.mod file so there should be one version scheme. This is really the wrong thing to be doing.

Beyond the confusion of having two streams of version progression, not using vMAJ.MIN.PAT goes against the go tooling for handling modules: https://golang.org/cmd/go/#hdr-Module_compatibility_and_semantic_versioning and https://research.swtch.com/vgo-principles

Spixmaster commented 3 years ago

I have never even touched go but even without any go principles I do not like their versioning for this project too. What I would do is just merge both projects into one. What is wrong with having one client desktop application for ProtonMail which includes all available features? The import-export software is something which is only used once. The software would only be downloaded to be used for a few minutes. Including it as an optional feature in ProtonMail Bridge would make more sense.

kortschak commented 3 years ago

From looking a the code it would reduce code duplication too,

Spixmaster commented 3 years ago

@jameshoulahan This might be a suggestion you should consider as a team.

jameshoulahan commented 3 years ago

I understand what you mean, @kortschak, but as a team we decided that since this repository is not intended to be used as a library (imported from other modules), this is essentially a nonissue -- go mod doesn't care what version its own module is. With this in mind, do you still think it's the wrong thing to be doing?

(off the record, I too would like to see one combined app and one valid-semver tagging system. But this way of doing things was arrived at via whole-team decision)

kortschak commented 3 years ago

I do think it's the wrong thing to do still. It adds complication for no real benefit, and while you say that is not intended to be used as a library, use as a library is required for rendering docs by pkg.go.dev (for those of us looking at reporting bugs this is important).

Also, when using the go tool to investigate the module version, because no valid semver is used, we see this

~ $ go version -m /usr/bin/protonmail-bridge
/usr/bin/protonmail-bridge: go1.13.15
    path    github.com/ProtonMail/proton-bridge
    mod github.com/ProtonMail/proton-bridge (devel) 
    dep     (devel) 
<snip>

Here you can see that module version is (devel) rather than the actual version (if the version were correctly semverised, there would be the version tag and a module hash) — as an aside, 1.13.15 is out of service life and there have been a number of bug fixes (including crypto and security fixes) since then.

kortschak commented 3 years ago

Note that currently pkg.go.dev only renders 1.3.3 and does this improperly because of the COPYING.md which it takes to be an unknown license.

kortschak commented 3 years ago

@Spixmaster Would you please reopen this. I don't believe that it has been resolved.

Spixmaster commented 3 years ago

@kortschak The reason I closed it is that they already stated that they will not change it. I do not see the reason of reopening the issue. I do not like their decision but it is made.

kortschak commented 3 years ago

Thanks. Yes, however @jameshoulahan asked for more information, though has not responded to that information having been provided.

kortschak commented 3 years ago

I should add that there is a correct way to do what the team have decided to do; <reposubpath>/v<semver> and add go.{mod,sum} at reposubpath/. This does what the team here feels is important to encode, but does so without breaking Go ecosystem module invariants. Use of this approach can be seen in the gopls path of the golang.org/x/tool module: https://github.com/golang/tools/tags

cuthix commented 3 years ago

After discussions we decided to again use tags in format vX.Y.Z this will correspond with version of bridge. You might notice that we also included tags for missing versions. Import-Export app versions will be tagged as they are ie-X.Y.Z. This should resolve all the issues.