build: use a more user-friendly version string for shallow clones #15792
Conversation
|
Download the artifacts for this pull request: |
common/meson.build
Outdated
| if is_git.returncode() == 0 and has_tag.returncode() != 0 | ||
| # put the mpv version and hash together | ||
| mpv_version = 'v' + meson.project_version().replace('-UNKNOWN', '') | ||
| hash = run_command([git_args, 'rev-parse', '--short', '@'], |
There was a problem hiding this comment.
this will only work if meson configure is run.
There was a problem hiding this comment.
Well yeah the hash doesn't update with new commits if you're doing development on a shallow clone but nobody does that right? Could write a python script I guess.
There was a problem hiding this comment.
It's less than 10 lines as an inline script so not too bad.
There was a problem hiding this comment.
It might be all in python to avoid mixing meson/python logic.
Dudemanguy
force-pushed
the
version
branch
3 times, most recently
from
9b1c83e to
e5697f1
Compare
|
I'm not a fan of
imho it's still not helpful. The information that it's is v0.39.0 is not valuable, when we are half a year in the future, with over 700 commits on top. |
|
Well, the stated rationale of the change is to fix software that is doing a version comparison to see if your mpv version is "new enough to support XXX features", no? Given that constraint, it doesn't matter how many commits you have on top. The stated objective can still be achieved. |
We never agreed to this contract, and the documentation clearly states that I have no objection to improving the displayed version, but it should remain purely informational and not be used for feature testing. The mpv API has a defined version in client.h Lines 250 to 251 in 38ad1ed As for the rest of client interface, each property or option operation will return an appropriate error code if it does not exist. We do not increment a "number" when interface changes are made, such as adding or removing properties or options. Instead, you can query whether your current mpv supports a given property.
It solves one specific scenario only. And ignores the fact that in those "many commits" properties may have been removed and you are blindsided. You are already using git version of the software, don't expect that v0.39.0 is the same as v0.39.0 + 723 commits... We have mechanism to check for properties availability. (or if you want to use new feature in your dev build, do you wait for v0.40.0 to be released?, so that your version check "works") Back to the PR changes itself, I find describe output way more clear |
|
I don't really think the commit number is meaningful personally. In fact, I had to look up what that part of the string meant from git describe since I didn't even know off the top of my head. But sure, I can just add 'dev' to the string to clear things up. |
... and clearly, anyone who wishes to retrieve information about what functionality is available in /usr/bin/mpv shall be required to do so by compiling a program against client.h? The application from the issue report is a javascript application btw, which tells users to download mpv.app from https://mpv.io/installation/ -- so it's not clear to me how you expect them to do feature testing with client.h even if they wanted to. It's not like the mpv.app downloadable bundle actually includes client.h or anything... |
javascript and lua interface don't need any version checking, you can test for existence of functions / properties / options. client.h exposes version, because it is important for building/linking C interface. |
common/meson.build
Outdated
| '--git-dir=' + join_paths(source_root, '.git'), | ||
| '--work-tree=' + source_root, | ||
| 'describe', '--always', '--tags', '--dirty'], | ||
| command: version_command, |
There was a problem hiding this comment.
I did some bikeshedding, feel free to use any if you like.
git = find_program('git', required: false)
generate_ver = '''
#!/usr/bin/env python3
import os
import sys
from subprocess import DEVNULL, check_output
git_cmd = [sys.argv[1], "--git-dir=" + os.path.join(sys.argv[2], ".git"),
"--work-tree=" + sys.argv[2]]
describe_cmd = git_cmd + ["describe", "--abbrev=9", "--tags", "--dirty"]
try:
ver = check_output(describe_cmd, stderr=DEVNULL, encoding="UTF-8")
except Exception:
describe_cmd += ["--always"]
ver = check_output(describe_cmd, stderr=DEVNULL, encoding="UTF-8")
ver = f"v{sys.argv[3]}-dev-g{ver}"
sys.stdout.write(ver)
'''
if not git.found()
git = ''
endif
version_h = vcs_tag(
command: [python, '-c', generate_ver, git, source_root, meson.project_version().replace('-UNKNOWN', '')],
fallback: 'v' + meson.project_version(),
input: 'version.h.in',
output: 'version.h',
replace_string: '@VERSION@',
)
sources += version_h
There was a problem hiding this comment.
btw. meson uses --dirty=+, not sure I like this very much, but it's more concise than default -dirty appended.
There was a problem hiding this comment.
If you insist on making it completely in python, that is fine but we should just reintroduce version.py at that point.
There was a problem hiding this comment.
I don't insist in making it in python. I would like to avoid it completely. Though even in your solution it is unavoidable.
I don't like the fragmentation of current solution. Particularly, we have describe command in 3 places. And rev-parse on top of that. Also checks run in meson may become stale, if say someone unhallow the repo.
I also want to make sure we have consistent naming in each case.
v0.39.0
v0.39.0-UNKNOWN // we could even replace our UNKNOWN with dev to be concise. I think UNKNOWN is bit obnoxious :) (why all caps?)
v0.39.0-dev-ge7852e984
v0.39.0-801-ge7852e984
v0.39.0-801-ge7852e984-dirty
And this also require --abbrev=9, which would need to be repeated 2 times again.
we should just reintroduce version.py at that point.
I see where you coming from, but it's still a simple wrapper over git describe. Also by doing it inline we can skip some checks of input arguments, because we know how it is called. Standalone script would be more bloated in arg parsing.
There was a problem hiding this comment.
Evidently, printing a number is a hard problem. An inline is OK if it is short and sweet but since we have to use try/except I am going to bring back version.py again. It's just more readable that way and there's yet another condition to control for if we're doing this in all in python.
That’s true but I’m fairly sure this check was added with 0.39 when the So yes, it can be done another way, but having to keep compatibility with two different syntaxes of the command that plays a file is an awkward situation. And when projects get a report that mpv 0.39 breaks their app, it is fairly obvious for them to just check the version string... and run into this issue a few months later. Also the C client API version can be queried from the JSON IPC (with get-version), but command changes are not tracked in the client API changelog, which is why it’s not really useful here. |
TOOLS/version.py
Outdated
| "--work-tree=" + sys.argv[2]] | ||
|
|
||
| try: | ||
| check_output(git_cmd + ["rev-parse"], stderr=DEVNULL, encoding="UTF-8") |
There was a problem hiding this comment.
I don't think this is necessary, describe will fail accordingly if needed.
There was a problem hiding this comment.
The exception will have an exception if git is not present but we can just early exit instead.
Edit: Actually no we can't because git could be there but it will still fail if it is a source tarball.
There was a problem hiding this comment.
That looks like a very complicated and exception-handler-laden way of writing this:
r = subprocess.run(git_cmd + ['rev-parse'], capture_output=True, text=True)
if r.returncode != 0:
sys.exit(1)
r = subprocess.run(git_describe, capture_output=True, text=True)
if r.returncode == 0:
ver = r.stdout
else:
r = subprocess.run(git_describe + ['--always'], capture_output=True, text=True)
if r.returncode != 0:
sys.exit('inconceivable! rev-parse succeeded, but git describe --always failed??')
ver = r.stdout
ver = f'v{sys.argv[3]}-dev-g{ver}'There was a problem hiding this comment.
two empty lines after imports
There was a problem hiding this comment.
That looks like a very complicated and exception-handler-laden way of writing this:
And what is better with dummy error code checks?
There was a problem hiding this comment.
Exceptions should be, well, exceptional.
There was a problem hiding this comment.
The exception will have an exception if git is not present but we can just early exit instead.
If there is exception (any) meson will fallback to fallback version.
Edit: Actually no we can't because git could be there but it will still fail if it is a source tarball.
What do you mean? If there is no git directory it will throw exception and meson will to fallback version.
See, you are optimizing for failure case, which is not what imho is the nominal case.
This dummy "early exit" rev-parse will force always at least 2 git commands to be executed. In case of shallow clone, it would be 3 commands. And in case when there is no git it would be 1.
What I propose is to skip rev-parse and let describe error out accordingly. This makes 1 single git command to be executed and at worse 2 if there are some errors, same in all other cases, shallow, non-git and so on.
There was a problem hiding this comment.
What I propose is to skip rev-parse and let describe error out accordingly
That doesn't work because a source tarball is not a git directory. The git describe will fail to the exception case which will fail again which floods the user terminal with traceback. I don't think you can disable the traceback in this case. Well maybe you can, but probably you shouldn't. At least two git calls are going to be needed.
There was a problem hiding this comment.
floods the user terminal with traceback
But that doesn't happen if you do it inline within meson so I guess you were right after all.
There was a problem hiding this comment.
Meson wraps this command and catch any abnormal command exit and fallbacks to fallback: 'v' + meson.project_version(),
The git describe will fail to the exception case which will fail again
the fail again case, can be fixed by more strict except ...: which would catch only if the command returned error, and pass through if command doesn't exist.
I don't think git returns different return codes for "not repository" or "no tags found" and also checking return codes is not really portable between posix and windows as the values will differ on the same logical errors.
TOOLS/version.py
Outdated
| #!/usr/bin/env python3 | ||
| import os | ||
| import sys | ||
| from subprocess import check_output, DEVNULL |
There was a problem hiding this comment.
two new lines after imports
Dudemanguy
force-pushed
the
version
branch
7 times, most recently
from
c5780a0 to
243a363
Compare
This was an inconsistency with the git tagging since the meson project version is just a number.
If the cloned repo is shallow (e.g. like the default github actions settings), the git describe command won't actually be able to fetch any tags and will instead just get a hash. The resulting binary version string will end up being that git hash. While it does tell you the exact commit, it's not exactly helpful when wanting to know the general version at a glance. For the case where we do have a git directory but no available tags, build a version string using the mpv version + the commit hash. Fixes mpv-player#15789.
|
Oh well, at least I discovered that our linter sucked in all of this mess. I'll merge kasper's suggestion soon. More than enough time was spent on this. |
Also for release tarball builds, put a 'v' in front of the number so it matches the formatting of the git tags.