ci: tarball name does not contain version

leamas commented 4 years ago

There are some reasons to include the version in the tarball filename:

Without version. it's not possible to store different version in a common namespace. While cloudsmith doesn't, most other usage scenarios will suffer from this.
Tools like ocpn-install.sh figures out version from the filename, and will fail without it.
FWIW, the tarball name is defined at https://github.com/leamas/opencpn/wiki/Tarballs#tarball-names

bdbcat commented 4 years ago

Hmmm.. I read at https://github.com/leamas/opencpn/wiki/Tarballs#tarball-names: "The release part is optional and can be omitted."

leamas commented 4 years ago

Yes. So version could be like 4.2.2 instead of 4.2.2-4.

I have understood this as you have forked someone else's radar_pi, and in that case the release makes sense. Otherwise not.

leamas commented 4 years ago

Confused... wrong plugin. release makes no sense here. I'll add a commit. sorry for the mess.

leamas commented 4 years ago

Dropped release part, pushed. Now building.

leamas commented 4 years ago

Just to clarify: The release part is used when for example Rick forks oesenc, adds a patch or two and publish. In this situation he cannot change the upstream version (i. e., your version), but still needs to differentiate his patched version.

So, it's basically a corner-case loophole, not often used.

rgleason commented 4 years ago

Dave, Is this going to affect the VDR_pi and Squiddio_pi plugins? Also I noticed that the VDR files came out with a difference of "VDR" and "vdr" which means that some of them will not be using the "CommonName" so those environments will not work right.

I wonder if the same thing is true for my version of squiddio? Mauro is still working on a new release with NDBC bouys and perhaps Ship's notices....

leamas commented 4 years ago

When I look into https://raw.githubusercontent.com/rgleason/plugins/rg-alpha/ocpn-plugins.xml, I see for example the following filenames as the last part of a tarball-url

vdr_pi-0.5.0-1_debian-10.tar.gz
squiddio_pi-1.2.1-1_darwin-10.13.6.tar.gz

Both of these are perfectly fine, they match the spec at https://github.com/leamas/opencpn/wiki/Tarballs#tarball-names. In particular, the filenames contains the version.

This issue is about oesenc_pi omitting the version in the filename, causing troubles as described in the top commit at https://github.com/bdbcat/oesenc_pi/issues/79#issue-585689425

rgleason commented 4 years ago

Thanks, good. Also I realized that although the Cloudsmith uploaded metadata and tarballs have "VDR" and "vdr" in the filenames, that is is not what is used for the "CommonName".

It is in the metadata you linked to, and that is all "VDR" as below, which is correct. Thanks.

 <plugin version="1">
        <name> VDR </name>
        <version> 1.2.0 </version>
        <release> 1 </release>

leamas commented 4 years ago

This is unrelated to this issue but you do have a point here. Since some tooling determines the plugin name from the filename it must match, also case-wise.

I will look into enhancing the check-metadata-urls tool to check also that the filename in the url complies with the spec.

leamas commented 4 years ago

@bdbcat: Please don't merge. I need to review this in light of Rick's comments

leamas commented 4 years ago

Hrmpf. I did get some things right, but not this. This is last-minute fix for a basic issue: the two names floating around for most plugins, like oesenc_pi (package name?) and oeSENC (plugin name as of GetCommonName()).

My initial attempt was to normalize all plugin names to use lowercase letters. However, we abandoned this, and I didn't see all the consequences.

My last commit in #80 (https://github.com/bdbcat/oesenc_pi/pull/80/commits/5fbfdb8634f) should clear things up w r t oesenc. However, this has some impact on all plugins.

I'll try to update the check-metadata-urls tool as a first step, but my time is limited.

That said, I think this is now ready to merge

bdbcat commented 4 years ago

Not to be difficult... And once more I remark that the current oesenc_pi name meets the spec, AFAICT.

So, as a plugin author, I ask rhetorically:

"What fails with my current naming scheme? It seems to work just fine so far..."
"And why do I care, if it currently working?"

Let us remember that all the plugin tooling is just that: tooling. If a plugin author wants to build a plugin by hand, we should not care what the name is, as long as the metadata is correct, and the URL points to a legitimate tarball, hosted at the author's choice of cloud or private storage.

rgleason commented 4 years ago

For plugin developers, many who are not that familiar with Cmake, it would be very good to have a PI Manager "Frontend #1" based upon oesenc_pi, that is working well, and documented clearly.

Part this requirement is making it clear what the CommonName is and what variable it goes into such that the result is a proper metadata.xml (verified).

Confirming these details will help ensure that plugin Devs will migrate.

bdbcat commented 4 years ago

CommonName has nothing to do with the title of this patch, and the issue at hand.

Also, let us not dance around the reallty. There are only 3 or 4 teams of plugin devs now. You know them all. And all are participating in the alpha work. Many of the older plugins will be adapted by you, or me, or Jon, or kees. I see no droves of early plugin devs awaiting a graven tablet....

rgleason commented 4 years ago

Ok

leamas commented 4 years ago

Not to be difficult...

Good questions are never difficult in that sense ;)

What fails with my current naming scheme? It seems to work just fine so far..."

The arguments are lined up in the top comment: https://github.com/bdbcat/oesenc_pi/issues/79#issue-585689425

And why do I care, if it currently working?"

A consistent naming scheme makes things easier for tools and deployment, and creates less cloudsmith lock-in.

ocpn-install (in plugins), the off-line installer, will fail if it cannot parse the plugin name and version from filename.

bdbcat commented 4 years ago

"ocpn-install (in plugins), the off-line installer, will fail if it cannot parse the plugin name and version from filename." That's the answer I was looking for.... But is it not more better to parse the metadata, and get the plugin name from there? I have not read the code....

leamas commented 4 years ago

For me, the overall difficulties storing files with same name but different version is quite a reason.

That said, when running ocpn-install the metadata is typacally not available. The alternative to parse the filename would be to ask for it, interactive or as an option. Neither seems that attractive.

leamas commented 4 years ago

Needed some sleep on this. Conclusions:

You are right about the naming and ocpn-install. It's better to parse the metadata which is available in any usecase involving this tool.

However, it's still problematic not having the version as part of the filename. It will make things complicated for most scenarios where these tarballs should be stored besides cloudsmith. Simply put, having two versions with the same filename will create collisions and confusion.

One example on this is the current cache implementation, which will happily use any version of oesenc cached when requested even though the request is for a specific version. As of now, this is not a problem with for example Rick's plugins; the cache logic is sound.

Note that this is just one example exposing the general problem of not encoding the version in the filename.

@bdbcat: thoughts?

bdbcat commented 4 years ago

Leamas No problem with the naming logic PR. I plan to merge soon, just as soon as I can surface from another rathole...

leamas commented 4 years ago

OK. Please hold until I clean up the PR, I'm in another rathole...

leamas commented 4 years ago

Cleaned up the PR. Assumptions:

It is not possible to reliably parse plugin name from file name. This need to be fixed in ocpn-install: https://github.com/OpenCPN/plugins/issues/60
It is still required that the filename contains the version so that different versions are unique as described in https://github.com/leamas/opencpn/wiki/Tarballs#tarball-names.

Late commits concerned with the (common) name part are dropped.

bdbcat / oesenc_pi

ci: tarball name does not contain version #79