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

ci: tarball name does not contain version #79

Closed
leamas opened this issue Mar 22, 2020 · 22 comments · Fixed by #80
Closed

ci: tarball name does not contain version #79

leamas opened this issue Mar 22, 2020 · 22 comments · Fixed by #80

Comments

@leamas
Copy link

leamas commented Mar 22, 2020

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

@leamas leamas mentioned this issue Mar 22, 2020
@bdbcat
Copy link
Owner

bdbcat commented Mar 22, 2020

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

@leamas
Copy link
Author

leamas commented Mar 22, 2020

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
Copy link
Author

leamas commented Mar 22, 2020

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

@leamas
Copy link
Author

leamas commented Mar 22, 2020

Dropped release part, pushed. Now building.

@leamas
Copy link
Author

leamas commented Mar 22, 2020

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
Copy link
Contributor

rgleason commented Mar 22, 2020

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
Copy link
Author

leamas commented Mar 22, 2020

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 #79 (comment)

@rgleason
Copy link
Contributor

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
Copy link
Author

leamas commented Mar 22, 2020

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
Copy link
Author

leamas commented Mar 22, 2020

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

@leamas
Copy link
Author

leamas commented Mar 22, 2020

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 (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
Copy link
Owner

bdbcat commented Mar 22, 2020

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:

  1. "What fails with my current naming scheme? It seems to work just fine so far..."
  2. "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
Copy link
Contributor

rgleason commented Mar 22, 2020

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
Copy link
Owner

bdbcat commented Mar 23, 2020

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
Copy link
Contributor

Ok

@leamas
Copy link
Author

leamas commented Mar 23, 2020

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: #79 (comment)

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
Copy link
Owner

bdbcat commented Mar 23, 2020

"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
Copy link
Author

leamas commented Mar 23, 2020

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
Copy link
Author

leamas commented Mar 25, 2020

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
Copy link
Owner

bdbcat commented Mar 25, 2020

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

@leamas
Copy link
Author

leamas commented Mar 26, 2020

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

@leamas
Copy link
Author

leamas commented Mar 26, 2020

Cleaned up the PR. Assumptions:

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

@bdbcat bdbcat closed this as completed in 7716d63 Mar 27, 2020
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 a pull request may close this issue.

3 participants