From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x235.google.com (mail-oi1-x235.google.com [IPv6:2607:f8b0:4864:20::235]) by sourceware.org (Postfix) with ESMTPS id 91C883858C74 for ; Tue, 8 Mar 2022 20:42:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 91C883858C74 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-oi1-x235.google.com with SMTP id j83so532076oih.6 for ; Tue, 08 Mar 2022 12:42:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=AOBUrljc5IN775i8FuBAI2yFuLXMs0c2Arixiid6HaU=; b=Wjl/89AQMASYfItpEOJ3xXGD70sRBxwz6hWQhj3DuMW6F230ed7n/VDUZBWe9tz5B0 v249iJ8f6QzwHr/HrzAqCqX9kzPCbXMyfYoOkF6RymdzZpS9dtiIYiFVPqAi7/XDnAnI rX5Pg9EYgwzXMVwCtEcgqOqRzgGuz8oDt38YvyfUjWnBWaygFSObABAvDiBlVU/VligX JdGzKIfJ3m2d9OLcb25/DfmdZsSVO0f9GDJozlt+RDfcY3OWCiQsC8h2LnrmnffNOSb1 cTyGU+qNyEKm8OXXdPDEkDQqvIx7WFmaQQdrfPgfNOu1GsQWKXSl3pxlOMXVyqEfvB00 m2+g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=AOBUrljc5IN775i8FuBAI2yFuLXMs0c2Arixiid6HaU=; b=zUKs7f9VukRv6usD9xpVYrfuzstxQBxiPHYkXJheNjmI+3mMjkCNfdf4mmNhHmuZh2 eC3+xh1rsufQ+7fK/YESxW3TJeQlgPBuXsjb59hKPYaro1FEM5v4bDR0jv+YFJ2+DfRB 50uk6JTX0JsYx8d3IQYa04PbXpillnWEkQr8h8LoUmw28k2s7/+9Hh5Ot27FgvOG2qUf 0EhDDViknWPl7awKdg+fLi4BTcBGrphJU//tEzgzyyDpi9YtcrWlVXRx7R8LEy7bht6E D9KlGbPWHzDgkGm6LuFcoJcrP6spindNf3SgL6qe2LWoeQEa6FNq8CZfIgxxYxvs3gmS AVCQ== X-Gm-Message-State: AOAM532HsPJy4IhTGZHKQvCimgUFwnHPkTqD9mySrBwoSQBnPU2egE6s jerDzCtAZ24pRiQHWgLd3Aj8WgmF0fT5ls+JjeHNB+hm X-Google-Smtp-Source: ABdhPJwO4eouiKC63gPokiGt1Cj1azEmZiBwLgNt8pF3Re3ZR2SRsiACw6Cecs8qS+VshNLfpfpXk2dx5y76ZfH9mis= X-Received: by 2002:a05:6808:221e:b0:2d9:a01a:48cf with SMTP id bd30-20020a056808221e00b002d9a01a48cfmr3854721oib.282.1646772144554; Tue, 08 Mar 2022 12:42:24 -0800 (PST) MIME-Version: 1.0 References: <20220308092804.vbhtazpcg4yl7dck@lucy.dinwoodie.org> In-Reply-To: <20220308092804.vbhtazpcg4yl7dck@lucy.dinwoodie.org> From: James Morris Date: Tue, 8 Mar 2022 15:42:13 -0500 Message-ID: Subject: Re: [ITP] git-filter-repo 2.34.0 To: cygwin-apps@cygwin.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-0.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM, KAM_NUMSUBJECT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: cygwin-apps@cygwin.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Cygwin package maintainer discussion list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 08 Mar 2022 20:42:27 -0000 Hi Adam, Thanks for the feedback! > - You've patched the shebang from `/usr/bin/env python3` to > `/usr/bin/python3`. To what end? /usr/bin/env is part of coreutils > for Cygwin, so there shouldn't be any risk that it won't be installed. > If someone has their own compiled python3 in /usr/local/bin, they'd > probably expect that to be used, so I don't think I'd change the > shebang unless there's some clear and specific reason for doing so. I am trying to prevent exactly what you described. git-filter-repo needs Python >=3.5 to function and we know that `/usr/bin/python3` is the correct version. Suppose a user installed Python 3.3 at `/usr/local/bin/python3`, now git-filter-repo will run with the wrong Python version and most likely break. This is what other distributions do when they distribute Python scripts and I'm fairly sure Debian explicitly calls this out in its policy. > - You're changing the shebang with both a patch file and with a line in > src_compile; you don't need to do both! I suspect this is an artefact > of how Cygport packages the source files, but AIUI the canonical way > to do this sort of patching with Cygport is to drop the sed line from > your .cygport file and just keep the patch file that gets generated. Yeah the patch file was automatically generated when I ran `cygport all` and I wasn't sure what to do with it. To me it seems silly to have a patch just to change the shebang line when `sed` works fine. I'll try removing `sed` to see what happens. > - You've set the category as both Devel and Python. IMO (I've not > checked what the general consensus on this is) this shouldn't be in > Python: it's a tool that happens to use Python, but I'd expect the > Python category to be for things that are specifically useful to > people doing Python dev, so things like libraries that can be usefully > imported in a Python module, or tools for debugging Python scripts. I > think this should only be in the Devel category. Yeah I initially didn't have it in the Python category, but then I thought about how other tools like bzr and mercurial are there so it seemed appropriate. Granted I didn't check if they also provided Python libraries, but I thought it made sense to put git-filter-repo in the Python category to maybe warn users that installing it would pull in Python. > - That said, I think ideally you'd also be packaging git_filter_repo.py, > which does provide a Python library that users can import. At that > point, this would unambiguously belong in both categories. I wasn't sure how to go about this since I didn't know if that would mean making a bunch of 'python3*-git-filter-repo' packages. Do you think I should make it importable, remove it from the Python category, or just leave it as is? > - You're installing the main script into /usr/bin. I think the > executable should probably be installed into /usr/libexec/git-core, > along with other Git executables. This is what `git --exec-path` > returns, and matches what's described in INSTALL.md. More generally, > I think you should be emulating one of the installation mechanisms in > INSTALL.md, either using the provided makefile, or following the > described steps in the "Manual Installation" section of that doc. Since git-filter-repo is a third-party command like git-lfs, I am of the opinion that it should go in `/usr/bin` rather than git's private `libexec`. That's also how Debian, Gentoo, Arch, and Homebrew package it. Also I believe my cygport file does pretty much follow the manual installation instructions except for the part about installing git_filter_repo.py (for the reasons I stated above). I would like to use the Makefile, however it has a number of things that make it difficult to work with in this context and from what I've seen basically none of the other packagers use it and instead opt to do the installation manually like I have. > - Currently Cygport can't run the test suite. Ideally it'd be able to; > it seems unlikely that there would be Cygwin-specific regressions in > this code, but it's not out of the question, and given upstream > provide a test suite, it seems a shame to not use it. I'll add that in. The package does currently report some errors with git 2.35 and it has been reported upstream (not by me) https://github.com/newren/git-filter-repo/issues/344. I look forward to getting this in Cygwin! -James