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

viewzoom and viewrotation inputs break API #92

Closed
schumaml opened this issue Jun 20, 2017 · 15 comments
Closed

viewzoom and viewrotation inputs break API #92

schumaml opened this issue Jun 20, 2017 · 15 comments
Assignees
Labels
type.Bug Something isn't working as intended

Comments

@schumaml
Copy link

Hi,

we noticed that you broke the libmypaint API without a change to any version numbers. Was this supposed to happen?

See
https://bugzilla.gnome.org/show_bug.cgi?id=783936

@odysseywestra
Copy link
Member

odysseywestra commented Jun 20, 2017

Libmypaint Master Branch is going to be unstable for a bit. One of our contributors is working on bringing new parameters to the brush engine which viewzoom and viewrotation was one of them.


On that note. @achadwick, I've had a note in the 1.4.x Development Project page about updating the ABI version number which another user pointed out awhile back. Do we need to make a habit of updating that every time we introduce a feature which may break other projects?

@mitchfoo
Copy link

When you change the behavior of a function, you are in fact creating a new library, and
you need to update the .so name. It would be nice (actually required for proper installability)
if unstable development used a new soname, changes the name of the .pc files etc.

@achadwick
Copy link
Member

achadwick commented Jun 20, 2017

The .so library version is configured in our configure.ac, using the libmypaint_abi_* macros.

The GNU docs tell us to bump on releases, and we make releases of libmypaint. Quoting the link above:

  1. Update the version information only immediately before a public release of your software. More frequent updates are unnecessary, and only guarantee that the current interface number gets larger faster.

Policy decision time. Do we consider this advice outdated for the purposes of continuous delivery? It's a guarded "yes" from me, btw., unless we can do what @mitchfoo suggested.

Patches to make libmypaint.pc spit out different library names for the development mainline are welcome too. I think that's reasonable. You can use libmypaint_api_prerelease to distinguish (don't use it as a suffix though, just detect non-blank and add a canned "-dev", for example).

OTOH (and this is why I want that patch) we can't insist on "revision" being bumped by each new commit that doesn't mess with an interface.

I reject any suggestion that each new behavioural change requires a new library basename, but I do not think @mitchfoo was saying that.

@achadwick achadwick self-assigned this Jun 20, 2017
@achadwick
Copy link
Member

Note that we already bake in the API major and minor version number.

/usr/lib/libmypaint-M.m.so.0.0.0

What about

/usr/lib/libmypaint-1.4-unstable.so.0.0.0

Which then becomes

/usr/lib/libmypaint-1.4.so.1.0.0      (CURRENT bump)

? Conditionally suffixing LIBMYPAINT_API_PLATFORM_VERSION with something to indicate ABI instability between releases is pretty easy to do.

@achadwick
Copy link
Member

The new code is in fact an API change in an incompatible manner, which forces the libmypaint API version to climb to 2.0.0 for the next release (Semantic Versioning rules should apply). I thought the next release would be 1.4.0, but it currently cannot be because things have changed incompatibly since 1.3.x on master.

@briend : is this what you intended? Would you be willing to backtrack on the API change at the cost of more work to MyPaint projects (and introduce a stroke_to_with_zoom_and_scale() or something), or should we plough on towards 2.0? If we do, could we introduce an extensible futureproof struct-based stroke_to() API for a hypothetical libmypaint 2.0? Since we're discussing making the Python side of this a namedtuple and all, that might be nice.

@achadwick
Copy link
Member

More reading from the Debian front that implicitly discourages bumping the soversion too frequently: https://www.debian.org/doc/debian-policy/ch-sharedlibs.html#s-sharedlibs-runtime. Each time we bump libmypaint_abi_current (a.k.a. the soversion), it causes a change of Debian package name.

Decision (thrashed out on the #gimp IRC channel, will be documented in a libmypaint VERSIONING.md)

tl;dr: API versions on master increment as needed for the upcoming release as and when features and changes are added. ABI versions are always updated immediately before each release, and only then.

  1. The master branch contains the future release's API version number. This version number is updated as and when new features and API changes are committed to the master branch. Do this by updating configure.ac's libmypaint_api_* macros as part of your commit. The reference point for these updates is the current stable release's version number.

    Rules for the API version number: libmypaint uses Semantic Versioning for its API version number.

    Note that the major and minor components of the API version number form part of the soname for binary library files used at runtime, but they are not part of the libmypaint.so symlink used by your compiler when building against libmypaint. That's normal.

  2. We use prerelease suffixes at the end of the API version number during active development to help you identify what you are building against. The API version number of a formal release normally does not contain any prerelease sufffix.

  3. For ABI versioning, libmypaint does exactly what the GNU docs say. We will always update some part of the ABI version number immediately before each public release. This is done by tweaking configure.ac's libmypaint_abi_* macros. ABI versions are only bumped immediately before a release, and maintain their own (somewhat weird) version sequence that's independent of the API version number.

    Rules for the ABI version number: refer to the libtool manual

@mitchfoo
Copy link

I didn't try to 100% follow your last comment because all the autofoo and libtool stuff fries
my brain, but you must not have a libmypaint.so, because the library's name is truly
"libmypaint-x.y". There isn't a "libgimp.so" symlink either, because binary incompatible
versions of the same lib have to be installable side-by-side, the library's name is libgimp-2.0,
libgimp-3.0 (always .0), encoding the ABI families (that are compatible only within themselves)
into the library name. That's the only way to make sure nobody can mess up at any point
(developer, builder, packager, user). They are different libraries because they are incomplatible,
so they must not share a symlink that may point to whatever.

@achadwick
Copy link
Member

@mitchfoo
It's a symlink, and this is the way libpng does it.

Give me a reference for that "must not", and I'll reopen.

@mitchfoo
Copy link

Libpng is from the stone age. If there were two different libpngs, with different API, and incompatible,
would you want a libpng.so symlink that points to a random one of the two?

I don't have a link, I'm just saying what GTK+, GIMP, GLib etc are doing it, and they all
guarantee perfect parallel installability.

@achadwick
Copy link
Member

@mitchfoo
Traditionally it's installed by the distro's -dev or -devel package, of which there can only be one for libpng. Debian lore here allows for multiple side-by-side dev versions.

"Perfect parallel installability". Please tell me what and what get to be installed in parallel. Two different majors? Two different minors withing the same major? The entire API version string?

(For no particularly well thought-out reason, libmypaint's "API version" format is <major>.<minor>, so that is likely to be the level of distinguish-ability you get.)

I'll consider it. Are you asking for a fresh /usr/include/libmypaintX.X and libmypaintX.X.pc for each minor "API version" bump? That would be roughly how libglib[-2.0] does it AFAICT. See the current libmypaint breakdown below.

It will be the devil's own sausage factory getting autotools to do this from what we have, and I'm not looking forward to it.

$ dpkg -L libmypaint-dev
/.
/usr
/usr/include
/usr/include/libmypaint
/usr/include/libmypaint/mypaint-brush-settings-gen.h
/usr/include/libmypaint/mypaint-brush-settings.h
/usr/include/libmypaint/mypaint-brush.h
/usr/include/libmypaint/mypaint-config.h
/usr/include/libmypaint/mypaint-fixed-tiled-surface.h
/usr/include/libmypaint/mypaint-glib-compat.h
/usr/include/libmypaint/mypaint-mapping.h
/usr/include/libmypaint/mypaint-rectangle.h
/usr/include/libmypaint/mypaint-surface.h
/usr/include/libmypaint/mypaint-tiled-surface.h
/usr/lib
/usr/lib/pkgconfig
/usr/lib/pkgconfig/libmypaint.pc
/usr/share
/usr/share/doc
/usr/share/doc/libmypaint-dev
/usr/share/doc/libmypaint-dev/copyright
/usr/lib/libmypaint.so

@achadwick achadwick added the type.Bug Something isn't working as intended label Jun 22, 2017
@achadwick achadwick reopened this Jun 22, 2017
@achadwick
Copy link
Member

From IRC:

<mitch> achadwick: i'm not asking for a different library name for minor versions, everything that is binary and source compatible can share a library name
<mitch> achadwick: but if libmypaint-1.4 and libmypaint-1.6 are binary/source incompatible, they need different .pc files, different include folders, different library names
<mitch> achadwick: and you would use the API version number for that name
<mitch> achadwick: do you make incompatible changes between minor numbers?
<mitch> if yes, you need /usr/include/libmypaint-1.X and libmypaint-1.X.pc and libmypaint-1.X.so
<mitch> -1.X would become part of the library name

shakaran pushed a commit to shakaran/libmypaint that referenced this issue Jun 22, 2017
Recent changes on master have changed the public API in a backwards-
incompatible way. The next release will therefore be 2.0.0, not 1.4.0.

Add a VERSIONING.md for reference.

Closes mypaint#92.
@briend
Copy link
Contributor

briend commented Jun 23, 2017

Sorry about all this. . . I really did not expect such a huge issue. There is only one other thing I hoped to do that would break stuff, which is add pen barrel rotation input, but I suppose that is going to wait until we have a new flexible way to send inputs to libmypaint (3.0?). Theoretically we could have dozens of input types. It'd be awesome if libmypaint could just accept any number of inputs without updating definitions, etc.

@odysseywestra
Copy link
Member

odysseywestra commented Jun 23, 2017

@briend I wouldn't worry too much about it. It was bound to happen since libmypaint is used by a number of projects. We just need to establish a guideline of when to update the API and how to Update the API. That way we avoid accidental breakage like we have Gimp right now.

@achadwick also what do we want to do about ABI? @QuLogic mentioning that we would need to bump that when I merged in #57.

@achadwick
Copy link
Member

@briend

a new flexible way to send inputs to libmypaint (3.0?)

IMO this would be an excellent 2.0 feature! The trick will be getting it working in a way that's introspectable, and which doesn't break API. Do do we implement our own opaque mapping type with strong private types (char *, double), or alter stroke_to() to take a GHashTable * argument, or what?

@odysseywestra
Documented! See VERSIONING.md.

  • API version number bumps should go in immediately on master.
  • At release time, we will be bumping the ABI version number too.

I am in the process of deciding where the version string goes in build products like the pkg-config .pc file and header locations, and how much of it should go there. Developers presumably are only interested in backwards compatibility when building new things.

If anyone's building directly against master and expects perfect stability, that's their problem. Third parties wanting stability are already supported by git tags and signed release tarballs.

@achadwick
Copy link
Member

achadwick commented Jun 25, 2017

See #94 for the way I'd like it to work. If everyone subscribed here could please examine that PR and make sure that it's sensible, and likely to work with their particular combinations, I'd be super grateful.

I have other work depending on this elsewhere, so I will try to merge it by Monday AM.

briend pushed a commit to briend/libmypaint that referenced this issue Sep 5, 2017
Recent changes on master have changed the public API in a backwards-
incompatible way. The next release will therefore be 2.0.0, not 1.4.0.

Add a VERSIONING.md for reference.

Closes mypaint#92.
briend pushed a commit to briend/libmypaint that referenced this issue Sep 5, 2017
This will allow side-by-side installations of different libmypaint builds
at the level of the minor version number.

Closes mypaint#92.
briend pushed a commit to briend/libmypaint that referenced this issue Dec 9, 2018
Recent changes on master have changed the public API in a backwards-
incompatible way. The next release will therefore be 2.0.0, not 1.4.0.

Add a VERSIONING.md for reference.

Closes mypaint#92.
briend pushed a commit to briend/libmypaint that referenced this issue Dec 9, 2018
This will allow side-by-side installations of different libmypaint builds
at the level of the minor version number.

Closes mypaint#92.
briend pushed a commit to briend/libmypaint that referenced this issue Dec 9, 2018
Recent changes on master have changed the public API in a backwards-
incompatible way. The next release will therefore be 2.0.0, not 1.4.0.

Add a VERSIONING.md for reference.

Closes mypaint#92.
briend pushed a commit to briend/libmypaint that referenced this issue Dec 9, 2018
This will allow side-by-side installations of different libmypaint builds
at the level of the minor version number.

Closes mypaint#92.
briend pushed a commit to briend/libmypaint that referenced this issue Jan 25, 2019
Recent changes on master have changed the public API in a backwards-
incompatible way. The next release will therefore be 2.0.0, not 1.4.0.

Add a VERSIONING.md for reference.

Closes mypaint#92.
briend pushed a commit to briend/libmypaint that referenced this issue Jan 25, 2019
This will allow side-by-side installations of different libmypaint builds
at the level of the minor version number.

Closes mypaint#92.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type.Bug Something isn't working as intended
Development

No branches or pull requests

5 participants