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

add libgap versioning, and use it to set GAP kernel version, i.e. gap_kernel_major/minor_version #3928

Closed
wants to merge 4 commits into from

Conversation

dimpase
Copy link
Member

@dimpase dimpase commented Mar 26, 2020

as discussed on Slack. Note that automatically divining cur:rev:age seems to be pretty much impossible, so it's all manual increment.

@dimpase dimpase requested a review from fingolfin March 26, 2020 11:30
@ChrisJefferson
Copy link
Contributor

I don't really like the idea of a third version type (we already have kernel major version and minor version, and GAP version).

The way libtool does versioning does seem annoying. We could just make the version kernelmajorversion*100+kernelminorversion, and the age kernelminorversion. That would lead to big versions of course.

@dimpase
Copy link
Member Author

dimpase commented Mar 26, 2020

This would throw away the compatibility window idea provided by "age", no?

@dimpase
Copy link
Member Author

dimpase commented Mar 26, 2020

assuming the kernel getting re-implemented on top of libgap, this number of different versions would go down...

@ChrisJefferson
Copy link
Contributor

No, when we increment kernelminorversion, it's backwards compatable (hopefully I've set the "age" in the right way), and when we increment kernelmajorversion, the kernel isn't backwards compatible.

It could be that sometimes we break backwards compatability for kernel extensions, but not libgap, but I don't think we want to try to seperate those two concepts.

@ChrisJefferson
Copy link
Contributor

I'm not aware of any plans to reimplement gap on top of libgap. Of course if someone wanted to put the work in, and it didn't make GAP worse for existing GAP users and developers, I wouldn't have a problem with it

@fingolfin fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: build system labels Mar 26, 2020
@fingolfin
Copy link
Member

fingolfin commented Mar 26, 2020

Chris, there are good reasons why the libtool versioning works like that, it makes it possible to support different OSes which have very different ways of versioning shared libraries (think: Linux, macOS, *BSD, Windows -- they all do this differently).

Still, i also don't want "yet another version". Which is why I explained on the mailing list and again on slack that the kernel version should then be derived from the libtool version, as follows:

  • kernel_major = libtool_current - libtool_age
  • kernel_minor = libtool_age

There is no need for libtool_revision, it can just stay 0 forever.

@ChrisJefferson
Copy link
Contributor

@fingolfin : That looks sensible. I don't mind if the libtool or the kernelversion are the "official" one, and we derive the other -- I think we have the same derivation in both directions

@dimpase
Copy link
Member Author

dimpase commented Mar 26, 2020

OK, I'll implement this.

@coveralls
Copy link

coveralls commented Mar 26, 2020

Coverage Status

Coverage remained the same at 84.302% when pulling ee918e7 on dimpase:master into bc65fe1 on gap-system:master.

@ChrisJefferson
Copy link
Contributor

Just for when you hit it, in Makefile.rules there is the line cp .libs/cyggap-0.dll $@ # FIXME: HACK to support kernel extensions which is going to break. Could I suggest just changing it to .libs/cyggap-*.dll (unless it is easy to put the right version in here). This is just a hack for cygwin. Technically this will break if someone builds to versions in the same cygwin, but I imagine that's very uncommon.

@dimpase
Copy link
Member Author

dimpase commented Mar 26, 2020

Could I suggest just changing it to .libs/cyggap-*.dll (unless it is easy to put the right version in here).

well, I'll do this, but I have my doubts - e.g. Sagemath builds libgap on Cygwin, and this cyggap-0.dll
sees to be a part of GAP executable, i.e. "another" libgap.

I'd appreciate this tested on Cygwin by running make install-libgap...

@fingolfin
Copy link
Member

We test Cygwin with each PR on AppVeyor CI, and that passed.

configure.ac Outdated Show resolved Hide resolved
@ChrisJefferson
Copy link
Contributor

Extra comment -- however we end up defining the ABI, at the moment how these values should be updated is documented in src/modules.h , around line 37 (I'm not claiming that is the best place for it to be). If you end up changing how the values should be updated, be sure to update the docs (and feel free to move it to their definition, but at least it should be correct).

@fingolfin
Copy link
Member

Yes with an artificial rule for maintaining the kernel versions it would also work. But we don't want that. I already explained three times now how we would like it.

@dimpase
Copy link
Member Author

dimpase commented Mar 27, 2020

well, the question is whether you like to introduce one extra rule for exising pair of parameters, or a complete new (not to you and me, but to some other devs) pair of parameters, which need to be bumped up using an extra rule, too.

The former might be less invasive.

@fingolfin
Copy link
Member

I stated what I want . I will not continue this pointless discussion. I already wasted more time on it than it would take to implement it right from scratch

@dimpase
Copy link
Member Author

dimpase commented Mar 27, 2020

I think it is a good idea to ask whether people mind switching to new versioning scheme, in particular as libgap is not really the main GAP part, and it's not clear why its version shoud drive the kernel version.

@ChrisJefferson
Copy link
Contributor

In practice, I think only me and @fingolfin update these numbers, and as long as I only have to edit one version, and not two independent versions, I'm happy. I also trust @fingolfin 's opinion on what's best for libtool.

@dimpase
Copy link
Member Author

dimpase commented Mar 27, 2020

@ChrisJefferson: In practice, there are dozens of downstream projects which would curse whoever make a switch to a new versioning scheme, they'd have to figure out, and adapt their work to. It's not the first time GAP would be making the life of downstream harder than necessary, for reasons not really convincing.

Why not play it nice?

@ChrisJefferson
Copy link
Contributor

I don't really see what we are "breaking". I don't think anyone else cares about GAP's kernel version outside kernel developers (and to them it will look the same anyway), and we don't current have a libgap version at all.

@dimpase
Copy link
Member Author

dimpase commented Mar 27, 2020

I don't really see what we are "breaking".
GAP kernel packages? Linux distributions that package GAP (and libgap)?

@ChrisJefferson
Copy link
Contributor

Wait, now I'm totally confused.

Based on what Max suggested, as I understand it we are still going to have gap_kernel_major_version and gap_kernel_minor_version. The only difference is now they will be derived from the libtool versions, which will start(in stable-4.11 as current=7, age=0, revision=0 and in master as current=8, age=0, revision=0.

They will be derived by:

gap_kernel_major_version = libtool_current - libtool_age
gap_kernel_minor_version = libtool_age

The gap_kernel_major_version will possibly skip some numbers, but that doesn't matter, as long as it is monotonically increasing, and goes up whenever we break backwards compatability, which will happen because whenever we do that we will increment libtool_current by 1 and reset libtool_age to 0.

@dimpase
Copy link
Member Author

dimpase commented Mar 27, 2020

The difference between what Max proposes and what I implement is that Max proposes to switch to new pair of inputs (libtool's current and age), and my implementation sticks with the old ones - both schemes result in the same gap_kernel_major_version and gap_kernel_minor_version.

Copy link
Contributor

@hulpke hulpke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reply as review was requested but I don't feel competent to review this in a meaningful way, since I do not understand what the difficulty here is. (I would simply have given it the same version number as GAP, but I realize that this actually would not work and is the problem to be resolved.) So no objection from me and I am happy to go with whatever more competent contributors chose.

@dimpase dimpase changed the title add libgap versioning add libgap versioning, and use it to set GAP kernel version, i.e. gap_kernel_major/minor_version Mar 28, 2020
@dimpase
Copy link
Member Author

dimpase commented Mar 28, 2020

an update to switch to libgap c:r:a versioning is to be added shortly.

@dimpase
Copy link
Member Author

dimpase commented Mar 29, 2020

The last commit changes the versioning to use libtool's (c:r:a) numbers as input.

By the way, different sources say different things about incrementing c, r, a, e.g. https://autotools.io/libtool/version.html says that r must be incremented always.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: build system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants