Commit e4c7fc23 authored by Salamandar's avatar Salamandar

Change logic to detect .git directory on repositories (or file on submodules)

parent 9cd0584e
Pipeline #131024 passed with stages
in 35 minutes and 29 seconds
......@@ -17,8 +17,8 @@ warnings = []
# git-version.h is already present and not generated if dist tarball
is_dist_tarball = run_command('python', '-c',
'import sys,os; sys.exit(0 if os.path.exists("git-version.h") else 1)'
).returncode() == 0
'import sys,os; sys.exit(0 if os.path.exists(".git") else 1)'
).returncode() == 1
################################################################################
# Project info
......
  • The absence of .git does not make it a dist tarball. There is also CVS tarball which distros might use to obtain master since it is generally faster to download it than git clone, even with --depth 1.

  • Actually is_dist_tarball may be badly named. It is more ! is_git_repository. In which case, we don't try to generate git-version.h (obviously would fail), nor do we regenerate INSTALL.

    As for distributions making CVS mirrors of git repositories, is it actually a thing? Which distributions do this? And did you encounter a concrete problem or you just imagine there could be a problem? If a real-life problem, which is it please? We can't fix things if we don't know what exactly is broken. :-)

  • Sorry, meant VCS tarball, not CVS; like https://gitlab.gnome.org/api/v4/projects/GNOME%2Fgimp/repository/archive.tar.gz?sha=370499676f7d323ab185bfa1d9141faa84016371. And yes, it is an actuall error – the build fails for me when built from the aforementioned tarball:

    meson.build:1435:2: ERROR: File git-version.h does not exist.
  • Basically, this commit assumes that when .git does not exist, git-version.h does, which breaks https://gitlab.gnome.org/GNOME/gimp/blob/370499676f7d323ab185bfa1d9141faa84016371/meson.build#L1435 when it is not the case.

  • Sorry, meant VCS tarball

    So it's like a tarball of the whole repository? So first of all, this is dirty. The whole point of inventing the dist commands in build systems was to be able to control the resulting archive (i.e. make sure it works well). I know that github and co. just decided some day that a release could just be an uncontrolled archive of a whole repo, but are we supposed to follow bad release procedures?

    So that's my first reaction: anyone making binaries with non-supported methods of our software should not expect it to work.

    Now my second reaction: we can still probably do something for this issue, but as you can expect git-version.h will contain useless information (we can't grab information from git if git info is not available! No magic here…). So this part of GIMP will be broken. Even more annoying for dev builds (which I imagine this is what we are talking about here?), since people won't have commit information to give us when they will report bugs.

    Conclusion: I'll make this work, but I will make a big warning in the configure step to say "that's not supported. Don't do this".

  • So it's like a tarball of the whole repository?

    Exactly.

    The whole point of inventing the dist commands in build systems was to be able to control the resulting archive (i.e. make sure it works well).

    I would have thought bootstrapping and caching, both of which distros like to do from scratch: https://wiki.debian.org/Autoreconf

    The idea is that if you get clean source tree, you should be able to build it, or else it is broken. Having build system look at files other than the project source code is frowned upon; it is only tolerated for development builds because purity would be too inconvenient there.

    So that's my first reaction: anyone making binaries with non-supported methods of our software should not expect it to work.

    Downloading the VCS tarball is reasonable optimization that our GitLab fetcher does. It should work on stable versions, since there the version should be obtained from meson.project_version() any way, no need to consult git.

    We could switch to generic git fetcher that allows shallow clone keeping .git (not supported by GitLab fetcher at the moment) for the dev versions but the build still should not crash in this case.

    Conclusion: I'll make this work, but I will make a big warning in the configure step to say "that's not supported. Don't do this".

    Sounds good to me.

  • I agree with @jtojnar : having a "raw" source directory with no git directory should ideally work.

    I also agree with you : I'll rename the variable is_git_repository. It will be much clearer.

    Edited by Salamandar
  • I agree with @jtojnar : having a "raw" source directory with no git directory should ideally work.

    No. Project have build rules. You can't just cater to the whims of any random third party who decides to go against them. Here it's pretty simple: our build rules are here to help us debug. There are 2 cases:

    1. you build GIMP from a release tarball (stable or development versions): we know which code you run because we tagged a particular commit with this release version.
    2. you build GIMP from a random repository commit (a non-release development version, often called "nightlies"), in which case we can still know what exact code you run because we are extracting the commit hash from your source code.

    Now if you build GIMP from a random commit but there is no repository, we just can extract nothing (no magic here: no git, we can't grab info out of thin air). Someone who will run this version and will report bugs (as far as we are concerned, reporting bugs is one of the main purpose of a dev build) will be only able to say from the About dialog: "this is version 2.99.1". This currently means over 4000 commits and we would have no easy way to say if this person is running a years-old dev code or the latest one (of course, we can try to infer depending on what features are available or whatnot, but our time is better used if we don't have to play guess games).

    I also agree with you : I'll rename the variable is_git_repository. It will be much clearer.

    Don't bother, I already did it yesterday. I just hadn't committed yet because I needed some sleep first. I'll review my code and push soon.

    Edited by Jehan
  • Jehan @Jehan

    mentioned in commit 8b506034

    ·

    mentioned in commit 8b506034

    Toggle commit list
  • Done.

    @jtojnar The kind of builds you want are now possible, but they are marked unsupported because we have no easy way to know what commit the build was made from. So you can do these if you really want to, but know we don't like them (especially if they are public builds).

    Also know that we are not against custom builds of course. In our studio, we work on custom builds with different trees for instance (commit hash would be unknown by others), but we take the responsibility of these builds.

    In your case, is it just for yourself or are you actually making public builds which a lot of people will download?

    commit 8b5060349a95ca552cc1afa9f9c6f4becc2fe7f6 (HEAD -> master, origin/master, origin/HEAD)
    Author: Jehan <jehan@girinstud.io>
    Date:   Thu Nov 21 12:21:08 2019 +0100
    
        meson: allow non-dist non-git builds but mark them unsupported.
        
        Reported by Jan Tojnar as a comment to commit e4c7fc23 that builds from
        simple archives of the repo contents (without the .git tree) are
        currently broken. Well this is normal, as we only support builds from
        release versions or from the repository where we can extract a git hash.
        Any other kind of nightly build can be from any commit out of thousands
        and is maintenance hell.
        
        To be nice though, let's unbreak such builds, but they will be clearly
        marked as unsupported (warning at configure time + the extract commit
        hash will now be labelled "unknown (unsupported)", which will be a
        visible string in About and on unstable version canvases).
        Basically do so at your own risk.
        
        Also generate INSTALL all the time (not sure why it was only generated
        in non-git case).
    
     meson.build | 45 ++++++++++++++++++++++++++++++---------------
     1 file changed, 30 insertions(+), 15 deletions(-)
  • @Jehan

    The kind of builds you want are now possible, but they are marked unsupported because we have no easy way to know what commit the build was made from.

    Thanks.

    Though, I think, when the version is stable (https://gitlab.gnome.org/GNOME/gimp/blob/8b5060349a95ca552cc1afa9f9c6f4becc2fe7f6/meson.build#L67), there should be no issue, as there is always only a single commit with that version.

    But yeah, for general commit it does not make sense to support them.

    In your case, is it just for yourself or are you actually making public builds which a lot of people will download?

    It is available for evaluation purposes for people who check out this PR and build from it: https://github.com/NixOS/nixpkgs/pull/67576

  • Though, I think, when the version is stable (https://gitlab.gnome.org/GNOME/gimp/blob/8b5060349a95ca552cc1afa9f9c6f4becc2fe7f6/meson.build#L67), there should be no issue, as there is always only a single commit with that version.

    It's true we could make an exception for this case. On the other hand, stable builds will always have official source tarballs, which will contain a proper git-version.h and everything necessary (they will be verified tarballs, known to work, build and run properly). Since your point was about having faster downloads, just download our official release tarball (which will be even faster than your gitlab on-the-fly generated tarballs by the way).

    Seriously on-the-fly tarballs are practical for tinkering, but are a terrible idea for release or distribution purpose. This is like releasing completely untested and untraced code.

    It is available for evaluation purposes for people who check out this PR and build from it: https://github.com/NixOS/nixpkgs/pull/67576

    Ok. I do hope that you will switch to properly traced code (i.e. official tarballs or git) for the official thing (i.e. before merging). I would not like seeing a whole bunch of people reporting bugs with no known source.

  • Something like might work:

    diff --git a/meson.build b/meson.build
    index 2a0191bcba..b8a693f184 100644
    --- a/meson.build
    +++ b/meson.build
    @@ -1435,28 +1435,30 @@ has_version_h = run_command('python', '-c',
       'import sys,os; sys.exit(0 if os.path.exists("git-version.h") else 1)'
     ).returncode() == 0
     if is_git_repository or not has_version_h
    +  git = find_program('git', required: false)
    +
       gitversion_h1 = vcs_tag(
         input : 'app/git-version.h.in',
         output: 'git-version.h.in.1',
    -    command: [ 'git', 'describe', '--always', ],
    +    command: [ git, 'describe', '--always', ],
         replace_string: '@GIMP_GIT_VERSION@',
    -    fallback: 'unknown (unsupported)',
    +    fallback: stable ? meson.project_version() : 'unknown (unsupported)',
       )
       gitversion_h2 = vcs_tag(
         input : gitversion_h1,
         output: 'git-version.h.in.2',
    -    command: [ 'git', 'rev-parse', '--short', 'HEAD', ],
    +    command: [ git, 'rev-parse', '--short', 'HEAD', ],
         replace_string: '@GIMP_GIT_VERSION_ABBREV@',
    -    fallback: 'unknown (unsupported)',
    +    fallback: stable ? meson.project_version() : 'unknown (unsupported)',
       )
       gitversion_h = vcs_tag(
         input : gitversion_h2,
         output: 'git-version.h',
    -    command: [ 'git', 'log', '-n1', '--date=format:%Y', '--pretty=%cd', ],
    +    command: [ git, 'log', '-n1', '--date=format:%Y', '--pretty=%cd', ],
         replace_string: '@GIMP_GIT_LAST_COMMIT_YEAR@',
    -    fallback: 'unknown (unsupported)',
    +    fallback: stable ? meson.project_version() : 'unknown (unsupported)',
       )
    -  if not is_git_repository
    +  if (not is_git_repository or not git.found()) and not stable
       # We create git-version.h but know it will be useless because we are
       # not in a git repository. Output a warning.
       git_warning = '''
    Edited by Jan Tojnar
  • Ok. I do hope that you will switch to properly traced code (i.e. official tarballs or git) for the official thing (i.e. before merging). I would not like seeing a whole bunch of people reporting bugs with no known source.

    Already switched to shallow clones. For stable releases, we always use tarballs when available. And people are instructed to use our issue tracker.

  • @jtojnar stable is not true for a single commit but for the whole branch (bugfixes and small features). As @Jehan says, without git information that would generate completely untraceable builds.

  • @jtojnar stable is not true for a single commit but for the whole branch (bugfixes and small features). As @Jehan says, without git information that would generate completely untraceable builds.

    Oh, you are right, I was thinking stable = (gimp_app_version_micro % 2 == 0) but it it is actually checking minor.

    Edited by Jan Tojnar
  • Something like might work:

    Sorry I really don't think we should allow this. This basically means "officially supported", and it's not. Even less for stable releases actually. We don't want to see binary builds around of people thinking they run "stable versions" made from funky tarballs (as said, a dist tarballs is not just an archive of the repo contents: it is an archive which was extracted, then built, then unit-tested, and possibly manually tested if the maintainer feels like it).

    So let's not go down this road of random automated (and unsafe) tarballs please. 🙂

    Already switched to shallow clones. For stable releases, we always use tarballs when available. And people are instructed to use our issue tracker.

    Then it's good. Continue like this. :-)

    stable is not true for a single commit but for the whole branch (bugfixes and small features).

    Actually we usually increment the micro version to being even in one commit and increment once more (back to unstable) in the next commit (unless some last-second error is discovered). So usually it's one commit. Still what I said earlier is still true. We don't support non-dist tarballs as an official build method for distribution.

    (edit: sorry was wrong, should have read what @Salamandar said above; he is right, we only check the minor version, so this change is even more out of the question).

    Edited by Jehan
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment