KhronosGroup / glTF-Blender-IO

Blender glTF 2.0 importer and exporter
https://docs.blender.org/manual/en/latest/addons/import_export/scene_gltf2.html
Apache License 2.0
1.48k stars 316 forks source link

Tests are broken #873

Closed julienduroure closed 4 years ago

julienduroure commented 4 years ago

Seems to be linked to the new 2.83 alpha build https://builder.blender.org/download/

luchiago commented 4 years ago

Hi, can I grab this one?

emackey commented 4 years ago

@luchiago Sure. master should use 2.83 Alpha for "blender28" testing, and blender-v2.82-release should use 2.82 Beta for "blender28" testing. It will require 2 separate PRs for the 2 branches.

luchiago commented 4 years ago

Ok. Where should I start looking?

emackey commented 4 years ago

I would start by looking for the last time this was upgraded, for example from 2.80 to 2.81. There should be something in the history that shows how this is done.

scurest commented 4 years ago

That would be #787, which was supposed to keep working the next time this changed >_>

emackey commented 4 years ago

So the reason that #787 stopped working is that there are two now:

$ curl -s https://builder.blender.org/download/ | grep -oe '[^\"]*blender-2\.8[^\"]*linux[^\"]*-x86_64[^\"]*'
/download/blender-2.82-1cb938ef2c92-linux-glibc217-x86_64.tar.xz
/download/blender-2.83-01d9a2f71b56-linux-glibc217-x86_64.tar.xz

Ideally, I would like master to use 2.83, and the blender-v2.82-release branch to use 2.82.

scurest commented 4 years ago

Yeah, | sort | tail -n1 should get the latest one for master.

On blender-v2.82-release, we could do the same thing, but add a 2 to the 2\.8 part of the pattern?

emackey commented 4 years ago

For now I'm going to lock the tests to the specific version needed. I think trying to make this guess the right version is not as reliable when there are multiple release branches going on. Going forward, when we create a new release branch, we should make sure the tests on master get bumped up to the next alpha version.

scurest commented 4 years ago

What makes it less reliable?

The reason I don't like that is because it means CI for all in-flight PRs breaks whenever the version changes. Everyone needs to either rebase or cherrypick the CI change.

emackey commented 4 years ago

If we stay ahead of the game, they shouldn't break. For example, when 2.83 moves from alpha to beta, a 2.84 alpha build will become available, but our builds will still be tested on 2.83 until we change it. We'll create the 2.83 release branch, and then bump master to 2.84.

The only time this should break is if we forget, during the entire beta program for a given release, to migrate master onto the next alpha release.

I'd rather have the tests break than have a script auto-select the wrong release to test against.

emackey commented 4 years ago

In-flight PRs shouldn't break, unless they've been in flight for longer than the length of the beta period for a given release. Such PRs should get master merged into them to bring them up to date.

Does this make sense?

scurest commented 4 years ago

In the 2.79 issue you said that the master branch here is going to only support the very latest Blender development version. But if that's true then you can't bump the version to test only at each release. If a breaking change to Blender 2.8(X+1) occurs while Blender 2.8X is still what's being tested against then you still will have selected the wrong release to test against, necessitating a manual "early" version bump.

emackey commented 4 years ago

The daily builds can (unfortunately) break things at any time. The test script isn't locked to a particular patch version, nor is it locked to "alpha" vs "beta" labeling. They always grab the most recent daily build for a given major/minor version (where 2 is major and 82 is minor for example), and ignore the patch version and hash.

So let's say #876 gets merged and master moves to look for 2.83 while the release branch is still using 2.82. If someone pushes a breaking API change into 2.83, the next time the tests run from master it will pull a daily build and the new API will be in force. A change should be pushed to our master to match what happened upstream in Blender. The 2.82 branch might still be going at that point, but its tests are still looking for daily builds of 2.82, so it will not get tested against the 2.83-only API change. This is important, as we want the version of the addon released with 2.82 to not make use of any 2.83-only APIs that showed up during alpha.

Generally I'm trying to treat this addon as a core part of Blender, and lock its release and testing branches directly to the corresponding Blender release branches that will ship it. I think the tests will still break sometimes (and I'm concerned about the 2.82rc1 phase, hopefully that goes smoothly but I'm not certain of that).

But in general I think having specific branches here tied to specific branches of development in Blender itself is the way to go.

scurest commented 4 years ago

Okay, I think I get it.

I would still | tail -n1 it btw, just for the extra safety rail.