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

Fix bootstrapping MSYS2 pacman (#11499) #12080

Merged
merged 3 commits into from
Jun 24, 2020
Merged

Fix bootstrapping MSYS2 pacman (#11499) #12080

merged 3 commits into from
Jun 24, 2020

Conversation

endrift
Copy link
Contributor

@endrift endrift commented Jun 24, 2020

MSYS2 pacman recently transitioned to using .zstd packages from .xz packages, but we bootstrap from a pacman that doesn't support those. We need to add some additional bootstrap steps before pacman works. Note that this conflicts with #12044, so one of us will have to edit their PR to match the other.

Describe the pull request

  • What does your PR fix? Fixes [poco:x64-mingw] build failure #11499

  • Which triplets are supported/not supported? Have you updated the CI baseline? These changes shouldn't affect different triplets since it uses an internal msys2 tree.

  • Does your PR follow the maintainer guide? To the best of my knowledge yes.

endrift and others added 2 commits June 23, 2020 19:46
MSYS2 pacman recently transitioned to using .zstd packages from .xz packages, but we bootstrap from a pacman that doesn't support those. We need to add some additional bootstrap steps before pacman works.
@ghost
Copy link

ghost commented Jun 24, 2020

CLA assistant check
All CLA requirements met.

@BillyONeal
Copy link
Member

Thanks for your contribution @endrift , can you make the CLA bot happy?

I think we should take this one rather than #12044 right now because this one unbreaks CI/PRs, and that one is still marked as draft.

@endrift
Copy link
Contributor Author

endrift commented Jun 24, 2020

CLA signed. And that sounds like a good start. I thought about adding a helper routine for invoking pacman instead of copying a bunch of stuff repeatedly. Also, errors aren't checked for yet, so the script could be a lot more robust than it is right now.

@BillyONeal
Copy link
Member

@endrift Thanks!

@JackBoosY
Copy link
Contributor

@emptyVoid Could you review this PR?

Thanks.

Copy link
Member

@vicroms vicroms left a comment

Choose a reason for hiding this comment

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

LGTM

@JackBoosY JackBoosY added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Jun 24, 2020
@PhoebeHui
Copy link
Contributor

VTK failures would be fixed via #12067

@JackBoosY
Copy link
Contributor

Needs merge this PR before merge #12067.

Copy link
Contributor

@emptyVoid emptyVoid left a comment

Choose a reason for hiding this comment

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

#11499 should had been fixed by #11443.

The dash part looks like a good workaround for the current MSYS2 disability (which is explained here msys2/MSYS2-packages#2022).

Comment on lines +101 to +106
# we need to update pacman before anything else due to pacman transitioning
# to using zstd packages, and our pacman is too old to support those
_execute_process(
COMMAND ${PATH_TO_ROOT}/usr/bin/bash.exe --noprofile --norc -c "PATH=/usr/bin;pacman -Sy pacman --noconfirm"
WORKING_DIRECTORY ${TOOLPATH}
)
Copy link
Contributor

@emptyVoid emptyVoid Jun 24, 2020

Choose a reason for hiding this comment

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

This part is not needed (see msys2/MSYS2-packages#1962 (comment)).

The same approach was first implemented in #11443 (merged the next day after #11499), and it stopped working the moment upstream changed compression scheme for some of pacman dependency packages to zstd, then we resorted to a workaround you can now see in #11443. The workaround was removed in #11810 as obsolete since upstream reverted core packages to xz.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're zstd again, at least when I tried earlier today.

Copy link
Contributor

@emptyVoid emptyVoid Jun 24, 2020

Choose a reason for hiding this comment

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

If you are referring to the errors seen in msys-pacman-*-err.log, those are for non-core packages.

At the moment vcpkg_acquire_msys silently ignores any errors in Acquiring MSYS2... region, as it does not use RESULT_VARIABLE of _execute_process function. Thus the core system upgrade fails, and pacman stays at some old version which is unable to install zstd packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we don't need those packages why not just update pacman instead of all the packages?

Copy link
Contributor

Choose a reason for hiding this comment

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

We do not update all the packages, we only do the core system upgrade so that any subsequent package installs would succeed. Some of the packages vcpkg ports require depend on the core packages, and the core packages could not be upgraded in the same shell with non-core installs.

Copy link
Contributor Author

@endrift endrift Jun 24, 2020

Choose a reason for hiding this comment

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

There's a pacman -Syu in there that I didn't add, so yes you do update all the packages. I can try removing it and seeing if any additional tests fail, but I'm not sure I should touch that.

Copy link
Contributor

@emptyVoid emptyVoid Jun 24, 2020

Choose a reason for hiding this comment

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

Yes, I'm referring to pacman -Syu too -- it actually updates the package database and makes a core system upgrade only (see MSYS2 Installation step #5). To upgrade all the packages an additional pacman -Su command is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yes, because of how MSYS2 works. I forgot about that. I'll try it without my first pacman -Sy pacman.

ports/openssl-unix/CONTROL Outdated Show resolved Hide resolved
@BillyONeal BillyONeal merged commit eede79e into microsoft:master Jun 24, 2020
@BillyONeal
Copy link
Member

Thanks so much for your help!

@endrift endrift deleted the endrift-pacman branch June 24, 2020 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[poco:x64-mingw] build failure
6 participants