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

Merge 2.x into master #583

Closed
wants to merge 75 commits into from
Closed

Conversation

rhansen
Copy link

@rhansen rhansen commented Apr 21, 2020

Merging a release branch into master has the following benefits:

  • git describe always prints a string that includes the version of the latest release (when master is checked out). This makes it possible to use version: git in the Snapcraft config.
  • Running git tag --contains <ID of bugfix on release branch> shows all versions that include the bugfix.
  • There is no need to cherry-pick a bugfix that was submitted on a release branch—just merge the release branch into master.
  • It lowers the chance of a regression. If a fix is made on a release branch but not merged (or cherry-picked) into master then the next release branch will not have the fix.

whitequark and others added 30 commits June 11, 2016 20:22
A previous attempt to fix this was done in 0128b86. However, it was
not rigorous. The added offset was dependent on font size and it
introduced an error into edit control positioning. Further, it is
irrelevant to non-workplanes.

After this commit, the workplane drawing code adds a fixed offset
instead. Also, the "tab" is enlarged to not overlap with #XY etc.
Also, rename confusing "Show Text Window" menu item.
Before this commit, quitting through menu would bring up the dialog
twice when declining to save the file.
Such resizing may well get us past OpenGL's maximum texture size
and break rendering.
This commit fixes a bug introduced in commit 0d7aa0a.
Before this commit, controls went wild on zoom ratios other than 1:1,
and the rendering wasn't too good either.
Before this commit, any available libpng or libfreetype would be
picked, e.g. from Mono.framework. After, homebrew and MacPorts
are prioritized.
This is required by e.g. Debian guidelines.
If SolveSpace crashes after the open, or hangs and is forcibly
killed, data would be lost. (I lost my data.) Instead, remove
autosave only in two cases: right after a successful save, or right
after a save is declined. This should be completely safe.
Before this commit, it never highlighted anything at all.
It was broken when chord tolerance calculation was overhauled.
Before this commit, no events would actually be received.
whitequark and others added 19 commits October 17, 2016 02:07
The crash was introduced in e2e9167; I have not noticed that some
of the code relies on past-the-size entities in GW.GS being empty.
SolveSpace 2.0 used the height of 'A' (i.e. cap height) to determine
the reference height.
SolveSpace 2.1 completely broke that during transition to Freetype,
and used something more or less random, by using FT_Set_Char_Size
with units_per_EM.
SolveSpace 2.2 attempted to fix that, but also used something more
or less random, by using FT_Request_Size with "unit" values.

Turns out that Freetype actually doesn't have a concept of cap height
at all. It is possible to extract it from the TT_OS2 table that is
present in some TrueType fonts, but it is not present in Microsoft
fonts (the msttcorefonts ones), and for those Linux fonts in which
it is present it doesn't appear very reliable.

So instead, use the height of 'A' instead, like version 2.0 did.
This has the advantage that it is quite bulletproof, and also matches
exactly what the old files are measured against.

One downside is that fonts without an 'A' glyph would not render.
We can deal with that when it becomes a problem.
SSurface::TriangulateInto first populates the mesh with triangles
that have no color, and then paints them, which confused the code
that detects if a mesh is transparent into thinking that all of them
are; and that broke the "draw back faces in red" feature, since it
is disabled for transparent meshes.
Before this commit, polylines got flattened but all other entities
got exported with the proper Z coordinate. After this commit, all
entities are exported with proper Z coordinate.

Also, instead of exporting LWPOLYLINE (2d only), POLYLINE (2d/3d)
is exported; as a bonus it is more compatible with 3rd party
software, since it is older.
This commit follows 99d6f1f, which enabled export of Z coordinate
by using POLYLINE instead of LWPOLYLINE. After this commit, only
the AcDb2dPolyline/AcDb2dVertex are used when exporting flat views,
which may improve compatibility with 2d design packages.
glibc defines a CHAR_WIDTH macro in limits.h since about 6.3.*.
This is apparently added as a part of ISO TS 18661-1:2014, which
I cannot read because it is not publicly available, and which covers
some sort of floating-point extensions. This is one of those changes
that should never have been done yet here we are.
@CLAassistant
Copy link

CLAassistant commented Apr 21, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 4 committers have signed the CLA.

✅ Evil-Spirit
✅ rhansen
❌ parport0
❌ xythobuz
You have signed the CLA already but the status is still pending? Let us recheck it.

@whitequark
Copy link
Contributor

Please don't do this, for several reasons.

  1. The 2.x branch is hopelessly obsolete and should never really be touched again. The Snap packages should be described as 3.0~gitXXXXX, not 2.anything.
  2. Bugfixes were first applied to master, then to release branch, not the other way around.
  3. This will likely cause issues with git bisect.

@whitequark whitequark closed this Apr 21, 2020
@whitequark
Copy link
Contributor

whitequark commented Apr 21, 2020

To be clear, we can take a look at the workflow improvements you're suggesting in the next release branch (if there will be one). But I would strongly prefer to not touch the existing 2.x branch, which is a remnant of an ad-hoc release process that has not been relevant for years.

@rhansen
Copy link
Author

rhansen commented Apr 21, 2020

Sounds good, though there are a couple of things I wanted to point out:

  • This doesn't touch the 2.x branch, only master.
  • This does not introduce any changes to master (the diff between the merge commit's first parent and the merge commit is empty).

@whitequark
Copy link
Contributor

whitequark commented Apr 21, 2020

Thanks for the expanded explanation above, I see your rationale more clearly now. Other than for git tag --contains, am I correct that with our workflow and versioning scheme, there would be no practical improvement?

@rhansen
Copy link
Author

rhansen commented Apr 21, 2020

Correct, although it is a prerequisite to switching your snapcraft.yaml to version: git if you wanted to. (Or you could create a tag like v3.0-alpha1 on master.)

@rhansen
Copy link
Author

rhansen commented Apr 21, 2020

A downside of the current snapcraft.yaml versioning scheme is it is non-increasing: 3.0~b55dac76 is newer than 3.0~e51fdf6f but version string comparison tools will believe it is older because the letter b comes before the letter e. On a Debian-based system (e.g., Ubuntu):

$ dpkg --compare-versions 3.0~b55dac76 gt 3.0~e51fdf6f && echo true || echo false
false

Using git describe to generate the version string avoids this problem via a "number of commits since the tag" part (the 687 in v2.3-687-g888ed334 means the 888ed334 commit is 687 commits past the v2.3 tag).

I don't think that Snapcraft version strings are subject to the same semantics as Debian package version strings, but it's probably good practice to follow something like the Debian version string semantics.

@whitequark
Copy link
Contributor

I understand your motivation for using version: git now. In principle, that makes sense. However, for Snaps, Snapcraft enforces a monotonically increasing revision order, so it is not an issue. For Debian packages, my understanding is that the packager would also use a monotonically increasing revision order that is specific for that particular package, so it is not an issue there either. (For a while I maintained a Debian package upstream, but then a Debian maintainer came and in fairly strong terms asked me to stop. Since that package and its nightly builds were a constant headache anyway I was happy to oblige.)

For context, the ~e51fdf6f part of the version string was never supposed to be compared. I added it a long time ago to the About dialog to make bug reports more useful. I think that was before I was aware of workflows using git describe. In semantic terms, all 3.0~ versions are pre-releases of 3.0 and have no particular ordering.

I have two concerns about switching to git describe.

  1. Unlike the current scheme, using it would break shallow checkouts, which are handy for CI servers and such.
  2. The "number of commits since the tag" part works well if you publish (pre-release) packages with this versioning scheme to a centralized package registry. In that case, you can maintain a single chain of commits in the way you proposed in this PR, and the number of commits since the tag is well-defined. But with something like SolveSpace, the purpose of the git hash in the version string is to clearly identify experimental builds, builds from various branches, third-party builds, etc, none of which are ordered with regards to the mainline development.

It seems to me like the disadvantages of using git describe outweigh the advantages. Perhaps we should more clearly emphasize that git hashes should not be compared? If I recall correctly, PyPI uses versions like 3.0.g<HASH> rather than 3.0.<HASH> to avoid this kind of problem--perhaps we should do that instead?

@rhansen
Copy link
Author

rhansen commented Apr 21, 2020

You are right that git describe doesn't work in a shallow clone—it requires cloning enough commit history to get the most topologically recent tag.

I looked into Snapcraft's version string a bit more and it looks like it's meant for human consumption, not machine consumption (unlike Debian package version strings). So the current version scheme is perfectly fine for Snap.

Perhaps we should more clearly emphasize that git hashes should not be compared?

I don't think that's necessary. Humans understand that hashes aren't ordered, and any package maintainer will know to munge the version string to avoid hash comparisons.

Speaking of munging version strings, how much do you care about consistency between Help->About and package versions? Right now a Debian package maintainer would need to generate a version string via git describe or a date-based scheme (e.g., 3.0~20200421.0-g45eb2468) in order to package up a pre-release version of SolveSpace. Because of this, the package version would not match Help->About.

If I recall correctly, PyPI uses versions like 3.0.g<HASH> rather than 3.0.<HASH> to avoid this kind of problem—perhaps we should do that instead?

That doesn't solve the Debian comparison problem—it would still try to compare ge51fdf6f with gb55dac76 and get the wrong answer. Adding a g prefix does have a few other advantages:

  • For humans, the g acts as a hint for humans that the following hash is an abbreviated Git commit ID.
  • For machine parsing:
    • The g makes it easier to write code that reliably extracts the commit ID from the version string.
    • The g helps avoid parsing ambiguity in case the hash looks like something else.
    • The g makes it easier to update tooling when switching from Git to some other version control system that also uses hashes to identify commits.

But with something like SolveSpace, the purpose of the git hash in the version string is to clearly identify experimental builds, builds from various branches, third-party builds, etc, none of which are ordered with regards to the mainline development.

Note that the output of git describe includes the abbreviated commit ID to disambiguate all of the commits that are exactly N away from the tag. For example, v2.3-687-g888ed334 refers to the commit with hash 888ed334.

Also, context matters. If the version string came from Snap or a Linux distribution then it would be clear what the string referred to. If the version string came from someone building their own personal branch then it wouldn't have much meaning, but that's also true with the current version scheme.

@whitequark
Copy link
Contributor

You are right that git describe doesn't work in a shallow clone—it requires cloning enough commit history to get the most topologically recent tag.

Another thing it doesn't work well with is git archive. (You already can't build SolveSpace from tarballs generated by it--for some incomprehensible reason this command is not able to package submodules--but I have some hope that one day it'll be fixed, and GitHub's tarball downloads would become useful.)

Because of this, the package version would not match Help->About.

That's not an issue. All I want is to be able to look up the hash to conclusively identify which commit in the repository it was built from (or a lack of such commit). If the About dialog text clearly identifies the build as coming from Debian (or at least, modified by a packager), that is all the better.

Also, context matters. If the version string came from Snap or a Linux distribution then it would be clear what the string referred to. If the version string came from someone building their own personal branch then it wouldn't have much meaning, but that's also true with the current version scheme.

Yes. Let me rephrase my earlier statement. I think that not having a "number of commits since tag" part at all is better than having one that would be sometimes missing (because of shallow checkouts), sometimes misleading (because of third party builds), and sometimes correct; there's nothing more frustrating than apparently helpful but in reality misleading metadata. If I have to disambiguate it with the commit hash to be sure, why bother having it at all?

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 this pull request may close these issues.

None yet

6 participants