[1/5] Makefile: drop builtins from MSVC pdb list
diff mbox series

Message ID 20200813145719.GA891370@coredump.intra.peff.net
State New
Headers show
Series
  • slimming down installed size
Related show

Commit Message

Jeff King Aug. 13, 2020, 2:57 p.m. UTC
Over the years some more programs have become builtins, but nobody
updated this MSVC-specific section of the file (which specifically says
that it should not include builtins). Let's bring it up to date.

Signed-off-by: Jeff King <peff@peff.net>
---
Given that nobody has mentioned this, it makes me wonder if anybody is
even using this part of the Makefile at all these days. Or maybe having
extra lines here isn't a problem (though it's also missing some entries,
like one for git-bugreport).

 Makefile | 2 --
 1 file changed, 2 deletions(-)

Comments

Taylor Blau Aug. 13, 2020, 3:04 p.m. UTC | #1
On Thu, Aug 13, 2020 at 10:57:19AM -0400, Jeff King wrote:
> Over the years some more programs have become builtins, but nobody
> updated this MSVC-specific section of the file (which specifically says
> that it should not include builtins). Let's bring it up to date.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Given that nobody has mentioned this, it makes me wonder if anybody is
> even using this part of the Makefile at all these days. Or maybe having
> extra lines here isn't a problem (though it's also missing some entries,
> like one for git-bugreport).

If having extra entries didn't cause a problem, I would suspect that it
was just that. But that missing entries *also* doesn't cause a problem,
I'd suspect that this section of the Makefile just isn't being used.

Of course, I'm not using it since I'm not on Windows, but maybe dscho or
Stolee would know if there are legitimate uses. Of course, if there
aren't, I'm favor of getting rid of this section entirely.

>  Makefile | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 65f8cfb236..271b96348e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2901,7 +2901,6 @@ ifdef MSVC
>  	# because it is just a copy/hardlink of git.exe, rather than a unique binary.
>  	$(INSTALL) git.pdb '$(DESTDIR_SQ)$(bindir_SQ)'
>  	$(INSTALL) git-shell.pdb '$(DESTDIR_SQ)$(bindir_SQ)'
> -	$(INSTALL) git-upload-pack.pdb '$(DESTDIR_SQ)$(bindir_SQ)'
>  	$(INSTALL) git-credential-store.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
>  	$(INSTALL) git-daemon.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
>  	$(INSTALL) git-fast-import.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
> @@ -2912,7 +2911,6 @@ ifdef MSVC
>  	$(INSTALL) git-remote-http.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
>  	$(INSTALL) git-remote-testsvn.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
>  	$(INSTALL) git-sh-i18n--envsubst.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
> -	$(INSTALL) git-show-index.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
>  ifndef DEBUG
>  	$(INSTALL) $(vcpkg_rel_bin)/*.dll '$(DESTDIR_SQ)$(bindir_SQ)'
>  	$(INSTALL) $(vcpkg_rel_bin)/*.pdb '$(DESTDIR_SQ)$(bindir_SQ)'
> --
> 2.28.0.573.gec6564704b
>
Thanks,
Taylor
Jeff King Aug. 13, 2020, 3:08 p.m. UTC | #2
On Thu, Aug 13, 2020 at 11:04:28AM -0400, Taylor Blau wrote:

> On Thu, Aug 13, 2020 at 10:57:19AM -0400, Jeff King wrote:
> > Over the years some more programs have become builtins, but nobody
> > updated this MSVC-specific section of the file (which specifically says
> > that it should not include builtins). Let's bring it up to date.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > Given that nobody has mentioned this, it makes me wonder if anybody is
> > even using this part of the Makefile at all these days. Or maybe having
> > extra lines here isn't a problem (though it's also missing some entries,
> > like one for git-bugreport).
> 
> If having extra entries didn't cause a problem, I would suspect that it
> was just that. But that missing entries *also* doesn't cause a problem,
> I'd suspect that this section of the Makefile just isn't being used.
> 
> Of course, I'm not using it since I'm not on Windows, but maybe dscho or
> Stolee would know if there are legitimate uses. Of course, if there
> aren't, I'm favor of getting rid of this section entirely.

I cc'd Jeff Hostetler, who added it. :)

I'm also pretty not-knowledgeable about Windows, but I think that
anybody using MSVC would do so through Visual Studio these days. And
that's being covered with recent cmake stuff. Or maybe I'm just
clueless. We'll see.

-Peff
Derrick Stolee Aug. 13, 2020, 4:37 p.m. UTC | #3
On 8/13/2020 11:08 AM, Jeff King wrote:
> On Thu, Aug 13, 2020 at 11:04:28AM -0400, Taylor Blau wrote:
> 
>> On Thu, Aug 13, 2020 at 10:57:19AM -0400, Jeff King wrote:
>>> Over the years some more programs have become builtins, but nobody
>>> updated this MSVC-specific section of the file (which specifically says
>>> that it should not include builtins). Let's bring it up to date.
>>>
>>> Signed-off-by: Jeff King <peff@peff.net>
>>> ---
>>> Given that nobody has mentioned this, it makes me wonder if anybody is
>>> even using this part of the Makefile at all these days. Or maybe having
>>> extra lines here isn't a problem (though it's also missing some entries,
>>> like one for git-bugreport).
>>
>> If having extra entries didn't cause a problem, I would suspect that it
>> was just that. But that missing entries *also* doesn't cause a problem,
>> I'd suspect that this section of the Makefile just isn't being used.
>>
>> Of course, I'm not using it since I'm not on Windows, but maybe dscho or
>> Stolee would know if there are legitimate uses. Of course, if there
>> aren't, I'm favor of getting rid of this section entirely.
> 
> I cc'd Jeff Hostetler, who added it. :)
> 
> I'm also pretty not-knowledgeable about Windows, but I think that
> anybody using MSVC would do so through Visual Studio these days. And
> that's being covered with recent cmake stuff. Or maybe I'm just
> clueless. We'll see.

We create PDBs for our microsoft/git installers, such as in this
recent build: [1]

[1] https://dev.azure.com/gvfs/ci/_build/results?buildId=18994&view=artifacts&type=publishedArtifacts

Unzipping, I see PDBs for the git-* files listed in this command,
minus the ones that don't exist. So if this line is what creates
the PDBs, then it is not erroring out when trying to create the
PDBs for commands that don't exist.

Thanks,
-Stolee
Jeff King Aug. 13, 2020, 5:40 p.m. UTC | #4
On Thu, Aug 13, 2020 at 12:37:40PM -0400, Derrick Stolee wrote:

> We create PDBs for our microsoft/git installers, such as in this
> recent build: [1]
> 
> [1] https://dev.azure.com/gvfs/ci/_build/results?buildId=18994&view=artifacts&type=publishedArtifacts
> 
> Unzipping, I see PDBs for the git-* files listed in this command,
> minus the ones that don't exist. So if this line is what creates
> the PDBs, then it is not erroring out when trying to create the
> PDBs for commands that don't exist.

Ah, thanks for the explanation. The good news is that this list should
be up to date after my series. ;)

-Peff
Johannes Schindelin Aug. 14, 2020, 2:18 p.m. UTC | #5
Hi Peff,

On Thu, 13 Aug 2020, Jeff King wrote:

> On Thu, Aug 13, 2020 at 11:04:28AM -0400, Taylor Blau wrote:
>
> > On Thu, Aug 13, 2020 at 10:57:19AM -0400, Jeff King wrote:
> > > Over the years some more programs have become builtins, but nobody
> > > updated this MSVC-specific section of the file (which specifically says
> > > that it should not include builtins). Let's bring it up to date.
> > >
> > > Signed-off-by: Jeff King <peff@peff.net>
> > > ---
> > > Given that nobody has mentioned this, it makes me wonder if anybody is
> > > even using this part of the Makefile at all these days. Or maybe having
> > > extra lines here isn't a problem (though it's also missing some entries,
> > > like one for git-bugreport).
> >
> > If having extra entries didn't cause a problem, I would suspect that it
> > was just that. But that missing entries *also* doesn't cause a problem,
> > I'd suspect that this section of the Makefile just isn't being used.
> >
> > Of course, I'm not using it since I'm not on Windows, but maybe dscho or
> > Stolee would know if there are legitimate uses. Of course, if there
> > aren't, I'm favor of getting rid of this section entirely.
>
> I cc'd Jeff Hostetler, who added it. :)
>
> I'm also pretty not-knowledgeable about Windows, but I think that
> anybody using MSVC would do so through Visual Studio these days. And
> that's being covered with recent cmake stuff. Or maybe I'm just
> clueless. We'll see.

We have a couple ways to build Git on Windows:

- the standard way, in a Git for Windows SDK (which is a slightly modified
  subset of MSYS2). This uses GCC and GNU make and all the things that you
  might suspect given Git's origins on Linux.

- the Visual Studio way, after running `make vcxproj` in Git for Windows'
  SDK. As part of the `vcxproj` target, the non-C parts of Git are
  generated and committed.

- the "new" Visual Studio way, after running CMake. The non-C parts are
  generated through CMake, which is nice because no Git for Windows SDK is
  required to build this from start to finish.

- the Visual C way, as championed by Jeff (and which was a prerequisite to
  working on the `vcxproj` target): in a Git for Windows SDK, use GNU make
  but replace GCC by MSVC (via the command-line): `make MSVC=1`.

While all four methods work, the only one that is regularly used with the
`make install` command is the first one.

The location you modified is in the `MSVC` part of `compat.mak.uname`,
i.e. in the Visual C part.

We originally had a tentative plan to eventually, maybe, build Git for
Windows using Visual C by default. However, it turned out that the
standard malloc in Visual C's runtime was tuned for other workloads than
Gits, and that nedmalloc performed better, and we originally could not get
nedmalloc to compile with a modern Visual C, so we stopped that effort.

That's why you still have support for `make MSVC=1 install`.

Since it seems not to be too much trouble, I would prefer to keep it
working for now, even if it is rarely exercised and regressions might
creep in (like the ones you fixed).

Thanks,
Dscho
Jeff King Aug. 14, 2020, 2:32 p.m. UTC | #6
On Fri, Aug 14, 2020 at 04:18:37PM +0200, Johannes Schindelin wrote:

> While all four methods work, the only one that is regularly used with the
> `make install` command is the first one.
> 
> The location you modified is in the `MSVC` part of `compat.mak.uname`,
> i.e. in the Visual C part.
> 
> We originally had a tentative plan to eventually, maybe, build Git for
> Windows using Visual C by default. However, it turned out that the
> standard malloc in Visual C's runtime was tuned for other workloads than
> Gits, and that nedmalloc performed better, and we originally could not get
> nedmalloc to compile with a modern Visual C, so we stopped that effort.
> 
> That's why you still have support for `make MSVC=1 install`.
> 
> Since it seems not to be too much trouble, I would prefer to keep it
> working for now, even if it is rarely exercised and regressions might
> creep in (like the ones you fixed).

Ah, thanks. That perfectly explains what is going on and why it might
not have been noticed. I agree it is not too much trouble to keep it
around for now.

-Peff
Johannes Schindelin Aug. 17, 2020, 4:42 a.m. UTC | #7
Hi Peff,

On Fri, 14 Aug 2020, Jeff King wrote:

> On Fri, Aug 14, 2020 at 04:18:37PM +0200, Johannes Schindelin wrote:
>
> > While all four methods work, the only one that is regularly used with the
> > `make install` command is the first one.
> >
> > The location you modified is in the `MSVC` part of `compat.mak.uname`,
> > i.e. in the Visual C part.
> >
> > We originally had a tentative plan to eventually, maybe, build Git for
> > Windows using Visual C by default. However, it turned out that the
> > standard malloc in Visual C's runtime was tuned for other workloads than
> > Gits, and that nedmalloc performed better, and we originally could not get
> > nedmalloc to compile with a modern Visual C, so we stopped that effort.
> >
> > That's why you still have support for `make MSVC=1 install`.
> >
> > Since it seems not to be too much trouble, I would prefer to keep it
> > working for now, even if it is rarely exercised and regressions might
> > creep in (like the ones you fixed).
>
> Ah, thanks. That perfectly explains what is going on and why it might
> not have been noticed. I agree it is not too much trouble to keep it
> around for now.

Incidentally, I stumbled across the patch I carry in Git for Windows'
branch thicket that _actually_ fixes this. Will send it out in a couple
minutes.

Ciao,
Dscho
Jeff Hostetler Aug. 17, 2020, 1:20 p.m. UTC | #8
On 8/14/20 10:18 AM, Johannes Schindelin wrote:
> Hi Peff,
> 
> On Thu, 13 Aug 2020, Jeff King wrote:
> 
>> On Thu, Aug 13, 2020 at 11:04:28AM -0400, Taylor Blau wrote:
>>
>>> On Thu, Aug 13, 2020 at 10:57:19AM -0400, Jeff King wrote:
>>>> Over the years some more programs have become builtins, but nobody
>>>> updated this MSVC-specific section of the file (which specifically says
>>>> that it should not include builtins). Let's bring it up to date.
>>>>
>>>> Signed-off-by: Jeff King <peff@peff.net>
>>>> ---
>>>> Given that nobody has mentioned this, it makes me wonder if anybody is
>>>> even using this part of the Makefile at all these days. Or maybe having
>>>> extra lines here isn't a problem (though it's also missing some entries,
>>>> like one for git-bugreport).
>>>
>>> If having extra entries didn't cause a problem, I would suspect that it
>>> was just that. But that missing entries *also* doesn't cause a problem,
>>> I'd suspect that this section of the Makefile just isn't being used.
>>>
>>> Of course, I'm not using it since I'm not on Windows, but maybe dscho or
>>> Stolee would know if there are legitimate uses. Of course, if there
>>> aren't, I'm favor of getting rid of this section entirely.
>>
>> I cc'd Jeff Hostetler, who added it. :)
>>
>> I'm also pretty not-knowledgeable about Windows, but I think that
>> anybody using MSVC would do so through Visual Studio these days. And
>> that's being covered with recent cmake stuff. Or maybe I'm just
>> clueless. We'll see.
> 
> We have a couple ways to build Git on Windows:
> 
> - the standard way, in a Git for Windows SDK (which is a slightly modified
>    subset of MSYS2). This uses GCC and GNU make and all the things that you
>    might suspect given Git's origins on Linux.
> 
> - the Visual Studio way, after running `make vcxproj` in Git for Windows'
>    SDK. As part of the `vcxproj` target, the non-C parts of Git are
>    generated and committed.
> 
> - the "new" Visual Studio way, after running CMake. The non-C parts are
>    generated through CMake, which is nice because no Git for Windows SDK is
>    required to build this from start to finish.
> 
> - the Visual C way, as championed by Jeff (and which was a prerequisite to
>    working on the `vcxproj` target): in a Git for Windows SDK, use GNU make
>    but replace GCC by MSVC (via the command-line): `make MSVC=1`.


On Windows, I always use either "make" or "make MSVC=1" from a SDK
shell window to build and test.  This lets me debug in either GDB or
Visual Studio directly against the EXE.  (That is, without any of the
project complications in the debugger or the artificial commits created
for the .vcxproj files.)  MSVC builds are built from source all the way 
down (including third-party libraries) thanks to vcpkg.

However, I never install my "make MSVC=1" builds, so I don't care about
the PDBs being redistributed.  As @dscho said, we added that thinking
that we would be able to do MSVC-based releases one day, but that ship
sank.

There were some very-early GVFS users who we (personally) gave MSVC-
based builds to help get stack traces via internal Watson channels,
but that is unnecessary now.

Jeff


> 
> While all four methods work, the only one that is regularly used with the
> `make install` command is the first one.
> 
> The location you modified is in the `MSVC` part of `compat.mak.uname`,
> i.e. in the Visual C part.
> 
> We originally had a tentative plan to eventually, maybe, build Git for
> Windows using Visual C by default. However, it turned out that the
> standard malloc in Visual C's runtime was tuned for other workloads than
> Gits, and that nedmalloc performed better, and we originally could not get
> nedmalloc to compile with a modern Visual C, so we stopped that effort.
> 
> That's why you still have support for `make MSVC=1 install`.
> 
> Since it seems not to be too much trouble, I would prefer to keep it
> working for now, even if it is rarely exercised and regressions might
> creep in (like the ones you fixed).
> 
> Thanks,
> Dscho
>

Patch
diff mbox series

diff --git a/Makefile b/Makefile
index 65f8cfb236..271b96348e 100644
--- a/Makefile
+++ b/Makefile
@@ -2901,7 +2901,6 @@  ifdef MSVC
 	# because it is just a copy/hardlink of git.exe, rather than a unique binary.
 	$(INSTALL) git.pdb '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(INSTALL) git-shell.pdb '$(DESTDIR_SQ)$(bindir_SQ)'
-	$(INSTALL) git-upload-pack.pdb '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(INSTALL) git-credential-store.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
 	$(INSTALL) git-daemon.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
 	$(INSTALL) git-fast-import.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
@@ -2912,7 +2911,6 @@  ifdef MSVC
 	$(INSTALL) git-remote-http.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
 	$(INSTALL) git-remote-testsvn.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
 	$(INSTALL) git-sh-i18n--envsubst.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-show-index.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
 ifndef DEBUG
 	$(INSTALL) $(vcpkg_rel_bin)/*.dll '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(INSTALL) $(vcpkg_rel_bin)/*.pdb '$(DESTDIR_SQ)$(bindir_SQ)'