Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add --include-pdbs to make_installers_from_mingw_w64_git() #403

Merged
merged 1 commit into from
Aug 2, 2021
Merged

Add --include-pdbs to make_installers_from_mingw_w64_git() #403

merged 1 commit into from
Aug 2, 2021

Conversation

vdye
Copy link
Collaborator

@vdye vdye commented Jul 29, 2021

Dependent on git-for-windows/build-extra#369 for --include-pdbs flag. Also removes separate bundle_pdbs step - not needed for this workflow.

References

@vdye vdye self-assigned this Jul 29, 2021
@vdye vdye requested review from derrickstolee and dscho July 29, 2021 20:48
@vdye vdye marked this pull request as ready for review July 29, 2021 20:48
Copy link
Collaborator

@derrickstolee derrickstolee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This draft release doesn't appear to have the PDBs. Am I missing something?

.github/workflows/build-git-installers.yml Outdated Show resolved Hide resolved
eval /usr/src/build-extra/please.sh make_installers_from_mingw_w64_git --version=${{ needs.prereqs.outputs.tag_version }} -o artifacts --${{matrix.artifact.name}} --pkg=pkg-x86_64/mingw-w64-x86_64-git-[0-9]*.tar.xz --pkg=pkg-x86_64/mingw-w64-x86_64-git-doc-html-[0-9]*.tar.xz &&
b="/usr/src/build-extra" &&
mkdir -p $b/cached-source-packages &&
cp pkg-x86_64/*-pdb* $b/cached-source-packages/ &&
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this line need to be after the eval?

Copy link
Collaborator Author

@vdye vdye Aug 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PDB archive needs to be "staged" in the cached-source-packages directory before make_installers_from_mingw_w64_git is run with the --include-pdbs flag (this pull request added that option: git-for-windows/build-extra#369).

EDIT: I'll add a comment clarifying that in the workflow

@vdye
Copy link
Collaborator Author

vdye commented Aug 2, 2021

This draft release doesn't appear to have the PDBs. Am I missing something?

After discussing it with @dscho, I learned that PDBs are included by embedding the PDB files in the installers themselves (the easiest way to tell is the size of the x86-64 Windows installer - it's about 48MB without PDBs (e.g., Git for Windows 2.32.0(2)) and about 52MB with PDBs (e.g., Microsoft Git 2.32.0.vfs.0.3 or the draft release). What I gathered from that was that including the separate archive of standalone PDB files wasn't necessary. I'm happy to add it back into the release, though!

@derrickstolee
Copy link
Collaborator

This draft release doesn't appear to have the PDBs. Am I missing something?

After discussing it with @dscho, I learned that PDBs are included by embedding the PDB files in the installers themselves (the easiest way to tell is the size of the x86-64 Windows installer - it's about 48MB without PDBs (e.g., Git for Windows 2.32.0(2)) and about 52MB with PDBs (e.g., Microsoft Git 2.32.0.vfs.0.3 or the draft release). What I gathered from that was that including the separate archive of standalone PDB files wasn't necessary. I'm happy to add it back into the release, though!

Ah! This is different than how I've been installing PDBs after-the-fact. Thanks for clearing this up!

Copy link
Member

@dscho dscho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we mark up the commit as a fixup! commit, so that future rebases are made a bit easier? Other than that, I like it!

@@ -187,24 +187,23 @@ jobs:
run: |
set -x

eval /usr/src/build-extra/please.sh make_installers_from_mingw_w64_git --version=${{ needs.prereqs.outputs.tag_version }} -o artifacts --${{matrix.artifact.name}} --pkg=pkg-x86_64/mingw-w64-x86_64-git-[0-9]*.tar.xz --pkg=pkg-x86_64/mingw-w64-x86_64-git-doc-html-[0-9]*.tar.xz &&
# Copy the PDB archive to the directory where `--include-pdbs` expects it
b="/usr/src/build-extra" &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The double quotes are not needed here because there is no special character nor space in the value. Maybe lose the quotes?

Copy link
Collaborator Author

@vdye vdye Aug 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed quotes & amended this commit to fixup! the Windows installer commit from the original pull request.

@vdye vdye merged commit 3d7b057 into microsoft:vfs-2.32.0 Aug 2, 2021
@vdye vdye deleted the feature/include-pdbs branch August 2, 2021 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants