From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-3.2 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,RCVD_IN_DNSWL_HI, T_RP_MATCHES_RCVD shortcircuit=no autolearn=ham autolearn_force=no version=3.4.0 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id 919621FADF for ; Mon, 8 Jan 2018 19:18:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932410AbeAHTST (ORCPT ); Mon, 8 Jan 2018 14:18:19 -0500 Received: from mail-qt0-f177.google.com ([209.85.216.177]:46592 "EHLO mail-qt0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932355AbeAHTSR (ORCPT ); Mon, 8 Jan 2018 14:18:17 -0500 Received: by mail-qt0-f177.google.com with SMTP id r39so14946084qtr.13 for ; Mon, 08 Jan 2018 11:18:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=CIqTwaOnI8VSuCNtn6o3rCVAyblDyyDs8gfaaIQEWo4=; b=oDXMK1DqUKqUzI5YS8MjgnwhOQCmpYrcQXaBOtW1hFOMtYwVukNGNgzvE4QMYp7RvV lMGk84KZom86ZrDkXbO3N2l/mFXmJW/w8AVcqXjA+IEyYww7ps1E3wBY/ECXRgb3Xj9/ +6y4eDkRZUCrBxCKp6UCWo+jaoUwycG2RWSF1NBQX/V/YExlPELjhck9RJ7vAqIqWz9a n0x3aprm5tawIzutsjQPL36N3AWtoSZcgW3XtnfJBULz+vX4O9zv9Dm4dvegRDyP1gve kOR7I3YflaBp+HF0V11g29CKjsjHhHVO23h/uboWwL5Y31udP37TxDdEQYwDhMSviAqH rHFQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=CIqTwaOnI8VSuCNtn6o3rCVAyblDyyDs8gfaaIQEWo4=; b=YNogx7BPikwKrV6iWr8DQH4BSwucKgSVn7pRyNFlt9aKA5TISI7hRBOIV8PXW1A+yw 573Q/2uQkHvykIktZQL95odGVd4ev6y8Iz5TtW/2I5tObXk+GQA1bm3hgeCQgbkEAD1k svvSw7RlkCxSdsarLh3L4R21f1waNAKGO9IYvHd0Z5bmi9svmjycF7RcopcMJ57WBVjH ul4vIe3vRjgrw+MW+XA5bDIYoJtiIFItHUMS6UHN9dGrLKItam8u2rpjD5wlunrYW/lC X4tNttDG9/DeSV/Wep2lkTp3+/XVqkz3o4k5mk6Q3HiKEw8sW9COTHzDlZjJVo5mOzCt bdIw== X-Gm-Message-State: AKwxytczgZc7rt7iU5ESkkGsm4QGUwJ9A6S5WW50xqwuJrQOUxruRB8n JiR2ZDOZvViM+0mmk3jqEUoulA== X-Google-Smtp-Source: ACJfBouMJSG4rRE9ueVFCgouCCWbwtD37go7SIWMpayIcucGZbKXYKsWUevlDa4wEQkf6JUrLHAkfQ== X-Received: by 10.200.17.25 with SMTP id c25mr16416251qtj.97.1515439096283; Mon, 08 Jan 2018 11:18:16 -0800 (PST) Received: from dnj-macbookpro.roam.corp.google.com.com ([172.23.222.209]) by smtp.gmail.com with ESMTPSA id k3sm7790025qtj.40.2018.01.08.11.18.15 (version=TLS1 cipher=AES128-SHA bits=128/128); Mon, 08 Jan 2018 11:18:15 -0800 (PST) From: Dan Jacques To: avarab@gmail.com Cc: Johannes.Schindelin@gmx.de, dnj@google.com, git@vger.kernel.org, gitster@pobox.com Subject: Re: [PATCH v5 2/3] Makefile: add Perl runtime prefix support Date: Mon, 8 Jan 2018 14:18:12 -0500 Message-Id: <20180108191812.52565-1-dnj@google.com> X-Mailer: git-send-email 2.15.0.chromium12 In-Reply-To: <87inccbscj.fsf@evledraar.gmail.com> References: <87inccbscj.fsf@evledraar.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Mon, 08 Jan 2018, Ævar Arnfjörð Bjarmason replied: > Thanks, applied this on top of next and it works for me, i.e. install to > /tmp/git and move to /tmp/git2 = works for me. Comments below. Good to hear! I've run this through a few machines at my disposal, but the more hands on the better. >> Enabling RUNTIME_PREFIX_PERL overrides the system-specific Perl script >> installation path generated by MakeMaker to force installation into a >> platform-neutral location, "/share/perl5". > > Not generated by MakeMaker anymore :) Hah good catch! I'll update the commit message. >>+# it. This is intentionally separate from RUNTIME_PREFIX so that notably Windows >>+# can hard-code Perl library paths while still enabling RUNTIME_PREFIX >>+# resolution. > > Maybe we covered this in previous submissions, but refresh my memory, > why is the *_PERL define still needed? Reading this explanation doesn't > make sense to me, but I'm probably missing something. > > If we have a system where we have some perl library paths on the system > we want to use, then they'll still be in @INC after our 'use lib'-ing, > so we'll find libraries there. > > The only reason I can think of for doing this for C and not for Perl > would be if you for some reason want to have a git in /tmp/git but then > use a different version of the Git.pm from some system install, but I > can't imagine why. The reason is entirely due to the way Git-for-Windows is structured. In Git-for-Windows, Git binaries are run directly from Windows, meaning that they require RUNTIME_PREFIX resolution. However, Perl scripts are run from a MinGW universe, within which filesystem paths are fixed. Therefore, Windows Perl scripts don't require a runtime prefix resolution. This makes sense because they are clearly functional right now without this patch enabled :) However, we don't have the luxury of running Perl in a separate universe on other OSes, so this patch is necessary for them. I created a separate option because I wanted to ensure that I don't change anything fundamental in Windows, which currently relies on runtime prefix resoultion. On all other operating systems, Perl and binary runtime prefix resolution is disabled by default, so if this patch set does end up having bugs or edge cases in the Perl runtime prefix code, it won't inpact anybody's current builds. I can foresee a future where Windows maintainers decide that PERL_RUNTIME_PREFIX is fine for Windows and merge the two options; however, I didn't want to force that decision in the initial implementation. > > + # GIT_EXEC_PATH is supplied by `git` or the test suite. Otherwise, resolve > > + # against the runtime path of this script. > > + require FindBin; > > + require File::Spec; > > + (my $prefix = $ENV{GIT_EXEC_PATH} || $FindBin::Bin) =~ s=${gitexecdir_relative}$==; > > So why are we falling back on $FindBin::Bin? Just so you can do > e.g. /tmp/git2/libexec/git-core/git-svn like you can do > /tmp/git2/libexec/git-core/git-status, i.e. will this never be false if > invoked via "git"? > > I don't mind it, just wondering if I'm missing something and we need to > use the fallback path in some "normal" codepath. Yep, exactly. The ability to directly invoke Perl scripts is currently functional in non-runtime-prefix builds, so enabling it in runtime-prefix builds seemed appropriate. I have found this useful for testing. However, since GIT_EXEC_PATH is probably going to be the common path, I'll scoop the FindBin code (including the "require" statement) into a conditional in v6 and use it only when GIT_EXEC_PATH is empty. > > + return File::Spec->catdir($prefix, $relpath); > > I think you initially got some version of this from me (or not), so this > is probably my fault, but reading this again I think this would be > better as just: > > return $prefix . '@@PATHSEP@@' . $relpath; > > I.e. right after this we split on @@PATHSEP@@, and that clearly works > (as opposed to using File::Spec->splitpath) since we've used it > forever. > > Better just to use the same idiom on both ends to not leave the reader > wondering why we can split paths one way, but need to join them another > way. PATHSEP is the path separator (":"), as opposed to the filesystem separator ("/"). We split on PATHSEP below b/c we need to "use lib" as an array, but it may be a ":"-delimited string.