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

[msys2] Bump version and support two profiles approach. #4286

Merged
merged 30 commits into from
Apr 25, 2021

Conversation

dmn-star
Copy link
Contributor

@dmn-star dmn-star commented Jan 17, 2021

Fixes #4277
Fixes #4355
Fixes #4066

Related to #4066
#4066 (comment)

Related to #1960
Related to #4301

After this date, we don't plan on building updated msys-i686 packages nor releasing i686 installers anymore. This is due to increasingly frustrating difficulties with limited 32-bit address space, high penetration of 64-bit systems and Cygwin (our upstream) starting their way to drop 32-bit support as well.
https://www.msys2.org/news/#2020-05-17-32-bit-msys2-no-longer-actively-supported

  1. Updating MSYS2 unattended is broken (MSYS2 locks up on full system upgrade) msys2/MSYS2-packages#1141
  2. [BUG] bash.exe crashed after upgrading msys2-runtime to version 3.1.4-1 msys2/MSYS2-packages#1966
  3. Pacman 5.2.1-7 breaks upgrade from older installation (e.g. appveyor) msys2/MSYS2-packages#1967
  4. https://www.msys2.org/news/#2020-05-31-update-may-fail-with-could-not-open-file
  5. https://www.msys2.org/news/#2020-05-22-msys2-may-fail-to-start-after-a-msys2-runtime-upgrade

Specify library name and version: msys2/all

  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@conan-center-bot

This comment has been minimized.

@ghost
Copy link

ghost commented Jan 17, 2021

I detected other pull requests that are modifying msys2/all recipe:

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@dmn-star
Copy link
Contributor Author

dmn-star commented Jan 17, 2021

msys2/20161025: Forced build from source
msys2/20161025 (test package): Installing package
Requirements
msys2/20161025 from local cache - Cache
Packages
msys2/20161025:eee3fba89db6d777329de604625af8c30d46f080 - Build
Build requirements
7zip/19.00 from 'conan-center' - Cache
Build requirements packages
7zip/19.00:ca33edce272a279b24f87dc0d4cf5bbdcffbc187 - Cache

Installing (downloading, building) binaries...
7zip/19.00: Already installed!
7zip/19.00: Appending PATH environment variable: C:\Users\dmn-star.conan\data\7zip\19.00__\package\ca33edce272a279b24f87dc0d4cf5bbdcffbc187\bin
msys2/20161025: Applying build-requirement: 7zip/19.00
msys2/20161025: Configuring sources in C:.conan\a684fd\1
msys2/20161025: Copying sources to build folder
msys2/20161025: Building your package in C:.conan\9a75aa\1
msys2/20161025: Generator txt created conanbuildinfo.txt
msys2/20161025: Calling build()
Downloading msys2-base-x86_64-20161025.tar.xz: 10%|9 | 4.39M/45.0M [00:32<04:56, 144kB/s]

@dmn-star
Copy link
Contributor Author

dmn-star commented Jan 17, 2021

ERROR: msys2/20161025: Error in build() method, line 64
self.run('bash -l -c "pacman -Syu --noconfirm"')
ConanException: Error 1 while executing bash -l -c "pacman -Syu --noconfirm"

msys2/MSYS2-packages#2058

No idea how you can fix the problem on the build machines :-)

We have prepared the following steps to verify and install the new keyring manually after which you should be able to use pacman -Syu again ...

see more at https://www.msys2.org/news/#2020-06-29-new-packagers

@dmn-star
Copy link
Contributor Author

@SSE4 can you please take a look at what's going wrong?

@SSE4
Copy link
Contributor

SSE4 commented Jan 19, 2021

@jgsogo as I remember, we had a situation when cross-building recipes were incorrectly discarded at the initial stage (export?) which was always done on Linux machine, so recipe incorrectly rejected all the configurations as valid. do you remember that case?

def configure(self):
if self.settings.os_build != "Windows":
def validate(self):
if not tools.os_info.is_windows:
Copy link
Contributor

Choose a reason for hiding this comment

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

that's probably the line discarding all the packages, because CI initially tries to export recipe on Linux to calculate all possible package IDs

@SSE4
Copy link
Contributor

SSE4 commented Jan 19, 2021

@dmn-star as I remember, CI works in two phases:

  1. execute only export, always on Linux machine, to understand how many different package_id recipe has
  2. execute individual jobs on actual agents

the check tools.os_info.is_windows probably rejects all the packages from the build.
I guess it's CI design bug, and you need to relax the check in order to allow the actual CI build to happen.

@jgsogo
Copy link
Contributor

jgsogo commented Jan 19, 2021

The CI works more or less as described: there is an initial export to check the HOOKS, and then in a Linux machine we run conan info for all the profiles to compute the different packageIDs and InvalidConfigurations. Then, we trigger a build for each of the profiles using the corresponding machine (Windows/Linux/Macos).

At this moment, all compilations in the CI are native ones, meaning that the machine running the build matches the profiles used for settings (host is equal to build machine in a cross-compiling terminology).

In this recipe, we probably want to check that self.settings.os == "Windows", because msys2 can only be generated for a Windows host. In general, we should avoid any check using platform, os_info,... which are related to the running machine, not to the package being generated.

In general, we should avoid any check using platform, os_info,... which are related to the running machine, not to the package being generated.
@conan-center-bot

This comment has been minimized.

@SSE4
Copy link
Contributor

SSE4 commented Jan 20, 2021

msys2/20190524: Applying build-requirement: 7zip/19.00
Traceback (most recent call last):
  File "c:\python36\lib\shutil.py", line 387, in _rmtree_unsafe
    os.unlink(fullname)
PermissionError: [WinError 5] Access is denied: 'C:\\J\\w\\cci_PR-4286@2/s\\b0d864\\1\\msys64\\usr\\bin\\dirmngr.exe'

@dmn-star
Copy link
Contributor Author

dmn-star commented Jan 20, 2021

This (?) error has also occurred sporadically on my workstation. 7zip/19.00 is more a "download" requirement.

msys2/20210105: Applying build-requirement: 7zip/19.00
ERROR: Cannot change permissions for C:.conan\577ccf\1\msys64\usr\bin\msys-2.0.dll!

@dmn-star
Copy link
Contributor Author

dmn-star commented Jan 20, 2021

Was 7zip/19.00 now compiled with msys shell and uses msys-2.0.dll?

Edit: No.
7zip_deps_conan

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot
Copy link
Collaborator

Failed in build 35:

WARN: Remotes registry file missing, creating default one in /home/conan/w/cci_PR-4286/35/e8fba80c-e3b0-4768-be0b-fefdf83b1cfc/.conan/remotes.json
ERROR: Error loading conanfile at '/home/conan/w/cci_PR-4286/recipes/msys2/all/conanfile.py': Unable to load conanfile in /home/conan/w/cci_PR-4286/recipes/msys2/all/conanfile.py
  File "/opt/pyenv/versions/3.7.5/lib/python3.7/site-packages/conans/model/ref.py", line 197, in loads
    name, version, user, channel, revision = get_reference_fields(text)
  File "/opt/pyenv/versions/3.7.5/lib/python3.7/site-packages/conans/model/ref.py", line 66, in get_reference_fields
    el1, el2 = _split_pair(arg_reference, "/") or (arg_reference, None)
  File "/opt/pyenv/versions/3.7.5/lib/python3.7/site-packages/conans/model/ref.py", line 18, in _split_pair
    raise ConanException("The reference has too many '{}'".format(split_char))
conans.errors.ConanException: The reference has too many '/'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/opt/pyenv/versions/3.7.5/lib/python3.7/site-packages/conans/client/loader.py", line 389, in _parse_conanfile
    loaded = imp.load_source(module_id, conan_file_path)
  File "/opt/pyenv/versions/3.7.5/lib/python3.7/imp.py", line 171, in load_source
    module = _load(spec)
  File "<frozen importlib._bootstrap>", line 696, in _load
  File "<frozen importlib._bootstrap>", line 677, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 728, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/home/conan/w/cci_PR-4286/recipes/msys2/all/conanfile.py", line 9, in <module>
    from ctypes import wintypes
  File "/opt/pyenv/versions/3.7.5/lib/python3.7/ctypes/wintypes.py", line 20, in <module>
    class VARIANT_BOOL(ctypes._SimpleCData):
ValueError: _type_ 'v' not supported


@conan-center-bot
Copy link
Collaborator

Failed in build 36:

WARN: Remotes registry file missing, creating default one in /home/conan/w/cci_PR-4286/36/e5241339-07b4-4041-b92f-49d47edc4380/.conan/remotes.json
ERROR: Error loading conanfile at '/home/conan/w/cci_PR-4286/recipes/msys2/all/conanfile.py': Unable to load conanfile in /home/conan/w/cci_PR-4286/recipes/msys2/all/conanfile.py
  File "/opt/pyenv/versions/3.7.5/lib/python3.7/site-packages/conans/model/ref.py", line 197, in loads
    name, version, user, channel, revision = get_reference_fields(text)
  File "/opt/pyenv/versions/3.7.5/lib/python3.7/site-packages/conans/model/ref.py", line 66, in get_reference_fields
    el1, el2 = _split_pair(arg_reference, "/") or (arg_reference, None)
  File "/opt/pyenv/versions/3.7.5/lib/python3.7/site-packages/conans/model/ref.py", line 18, in _split_pair
    raise ConanException("The reference has too many '{}'".format(split_char))
conans.errors.ConanException: The reference has too many '/'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/opt/pyenv/versions/3.7.5/lib/python3.7/site-packages/conans/client/loader.py", line 389, in _parse_conanfile
    loaded = imp.load_source(module_id, conan_file_path)
  File "/opt/pyenv/versions/3.7.5/lib/python3.7/imp.py", line 171, in load_source
    module = _load(spec)
  File "<frozen importlib._bootstrap>", line 696, in _load
  File "<frozen importlib._bootstrap>", line 677, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 724, in exec_module
  File "<frozen importlib._bootstrap_external>", line 860, in get_code
  File "<frozen importlib._bootstrap_external>", line 791, in source_to_code
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/home/conan/w/cci_PR-4286/recipes/msys2/all/conanfile.py", line 10
    except ImportError, ValueError:
                      ^
SyntaxError: invalid syntax


@conan-center-bot
Copy link
Collaborator

Some configurations of 'msys2/20210105' failed in build 37 (36487658817c6bead2189a88921f9306aaf018ab):

@conan-center-bot
Copy link
Collaborator

Failure in build 38 (471d40ee428e62c2aecc9ef38f2f2a2d243f4da1):

  • msys2/20161025:
    • Zero packages generated. Looks like all configurations have been discarded by ConanInvalidConfiguration exceptions

- drop version 20161025
- use new conandata.yml format
- avoid unnecessary copying
@conan-center-bot
Copy link
Collaborator

All green in build 39 (364fe93f98634f55ad606cab258724011cddbc70)! 😊

@SSE4
Copy link
Contributor

SSE4 commented Mar 3, 2021

@dmn-star finally, green
we have found it out - locally we always run just single build at time, and it's safe.
CI runs multiple builds in parallel on single machine.
but it runs taskkill for pacman, which is system wide.
so one CI job may abort another CI job in the middle of package installation.
as soon as first job is aborted abnormally, CCI cancels all other unfinished job.

Comment on lines +65 to +66
if tools.Version(self.version) <= "20161025":
raise ConanInvalidConfiguration("msys2 v.20161025 is no longer supported")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this required since the version was removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

if you don't remove it, you will receive zero packages generated error for version 20161025
but right now we're not sure these versions make sense at all, as we upgrade them right away, so effectively all the versions are the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually we are waiting for a decision regarding rolling releases. :-) cc @jgsogo

@jgsogo
Copy link
Contributor

jgsogo commented Mar 5, 2021

Hi! We've been talking about msys2, there are two main topics: versioning and the kill pacman command.

I think we all agree that msys2 is a Conan package because it is a tool we need to build other recipes, it is very unlikely that someone could use this package as a library. As such tool, we declare it as a build_requires and it is really an advantage for Windows users that are using Conan to be able to build from sources some libraries without the need to set up a complex development environment. There is a clear value in having msys2 available as it is now.

We are not using just msys2, but the packages installed inside. Right now the msys2 package is ready to use (for our requirements), our jobs using it as a build_require don't need to upgrade and install m4, make,... everything is already packaged during the build process. This is a huge advantage, we don't need to install again and again the same packages into msys2 (saving time and better UX), but OTH we face the issue related to kill pacman and the versions are misleading.

The version is misleading because, as said in previous comments, the packages installed by pacman are the ones when the reference was built (and it is built whenever there is a new recipe-revision), not the ones matching the date of the msys2 version. Only the sources used to compile msys2 match the date. Any commitment about reproducibility regarding this package can be related to the date of the recipe-revision, but not to the revision itself. And given that we package msys2 with all the tools installed, it is possible to reproduce a scenario using revisions (just fetch the revision from the date to reproduce and run without installing/upgrading anything with pacman). This reproducibility won't be possible if we were packaging a vanilla msys2 and every recipe was upgrading/installing before using it.

Now we have a hack to workaround the issue about killing pacman, for sure it is something that we should try to avoid. Maybe it is possible to get the pacman's PID for the msys2 we are building and kill only that one, or probably the best approach is to add something to config.yml that the CI will read and will avoid parallelization while building these packages. Anyway, the current lock proposal works.


What's next?

  • Let's keep the lock, it works. We will evaluate if it is easier to implement it on the CI side.
  • Let's have only one rolling release:
    • We need to decide the name for that version: cci.latest? <latest-sources-date>? All alternatives will have pros and cons, it is time to brainstorm them.

cc/ @danimtb @SSE4 @uilianries
contributors to this recipe, cc/ @vvilpas @madebr

@conan-center-bot
Copy link
Collaborator

All green in build 40 (364fe93f98634f55ad606cab258724011cddbc70):

  • msys2/20190524@:
    All packages built successfully! (All logs)

  • msys2/20200517@:
    All packages built successfully! (All logs)

  • msys2/20210105@:
    All packages built successfully! (All logs)

@SSE4
Copy link
Contributor

SSE4 commented Mar 11, 2021

hi @dmn-star
we have discussed it again and came up with the following conclusion:

  • cci.latest could be used to indicate a rolling release
  • 20210105 could be used as a current source, and older sources (20161025/20190524/20200517) could be dropped from the config.yml/conandata.yml
  • 20210105 source could be changed in future, as soon as new msys2 releases are coming (it means config.yml will always have just single entry to the latest source)
  • it's okay to use 20210105 now and always update to the latest packages available
  • it's okay to clean up the code to remove workarounds for older base sources, if you want

besides that new details, everything from Javier's previous comment is still valid

@stale
Copy link

stale bot commented Apr 10, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 10, 2021

# https://github.com/msys2/MSYS2-packages/issues/1966
def _kill_pacman(self):
if (self.settings.os == "Windows"):
Copy link
Contributor

Choose a reason for hiding this comment

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

This test can be dropped, it is already checked for in validate.

Copy link
Contributor

@madebr madebr left a comment

Choose a reason for hiding this comment

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

I think this pr has fixed its original intentions:

  • add new version: 20210105
  • support two profiles approach

Modifying the recipe to add rolling releases would make this pr drag on.
It's currently stalled anyways.

@prince-chrismc
Copy link
Contributor

prince-chrismc commented Apr 25, 2021

@Stale[bot] Remove the label so my bot can pick this up!

Will CCI merge a PR that's marked stale? does this require manual intervention?

@conan-center-bot conan-center-bot merged commit 8adb93a into conan-io:master Apr 25, 2021
AndreyMlashkin pushed a commit to AndreyMlashkin/conan-center-index that referenced this pull request Apr 26, 2021
* Bump version and support two profiles approach.

https://docs.conan.io/en/latest/reference/profiles.html#build-profiles-and-host-profiles

* PR 4286: invalid syntax

* PR 4286: add new version (20210105)

* PR 4286: CR avoid any check which are related to the running machine

In general, we should avoid any check using platform, os_info,... which are related to the running machine, not to the package being generated.

* PR 4286: try to move the msys2 download into source(.)

https://docs.conan.io/en/latest/reference/conanfile/methods.html#source Please read the Note

* PR 4286: 7zip ->  tools.unzip(.)

* PR 4286:  install the new keyring

 # https://www.msys2.org/news/#2020-06-29-new-packagers

* PR 4286: skip msys2 v.20161025

* PR 4286: fix invalid syntax

* PR 4286: try to kill pacman first

* PR 4286: try to kill pacman first on windows only

* Revert "PR 4286: try to kill pacman first on windows only"

This reverts commit e774845.

* Revert "PR 4286: try to kill pacman first"

This reverts commit 0048efa.

* PR 4286: pac-man workarounds

* PR 4286: Always kill all running msys2 processes

* PR 4286: remove configure

* PR 4286: more kills

* PR 4286: add --ask 20

* PR 4286: less kills

* PR 4286: skip hook warning

* - more logs

* - even more logs

Signed-off-by: SSE4 <[email protected]>

* - wait for the process

Signed-off-by: SSE4 <[email protected]>

* - it's possible we have several CI jobs which may incorrectly terminate each other with kill pacman

Signed-off-by: SSE4 <[email protected]>

* - guard to pass export on Linux

Signed-off-by: SSE4 <[email protected]>

* - typo

* - ValueError

* - syntax

* - don't raise

* - move downloads to build
- drop version 20161025
- use new conandata.yml format
- avoid unnecessary copying

Co-authored-by: SSE4 <[email protected]>
ericLemanissier added a commit to ericLemanissier/conan-center-index that referenced this pull request Apr 26, 2021
ericLemanissier added a commit to ericLemanissier/conan-center-index that referenced this pull request Apr 26, 2021
@ericLemanissier ericLemanissier mentioned this pull request May 4, 2021
4 tasks
AlvaroFS pushed a commit to AlvaroFS/conan-center-index that referenced this pull request May 7, 2021
* Bump version and support two profiles approach.

https://docs.conan.io/en/latest/reference/profiles.html#build-profiles-and-host-profiles

* PR 4286: invalid syntax

* PR 4286: add new version (20210105)

* PR 4286: CR avoid any check which are related to the running machine

In general, we should avoid any check using platform, os_info,... which are related to the running machine, not to the package being generated.

* PR 4286: try to move the msys2 download into source(.)

https://docs.conan.io/en/latest/reference/conanfile/methods.html#source Please read the Note

* PR 4286: 7zip ->  tools.unzip(.)

* PR 4286:  install the new keyring

 # https://www.msys2.org/news/#2020-06-29-new-packagers

* PR 4286: skip msys2 v.20161025

* PR 4286: fix invalid syntax

* PR 4286: try to kill pacman first

* PR 4286: try to kill pacman first on windows only

* Revert "PR 4286: try to kill pacman first on windows only"

This reverts commit e774845.

* Revert "PR 4286: try to kill pacman first"

This reverts commit 0048efa.

* PR 4286: pac-man workarounds

* PR 4286: Always kill all running msys2 processes

* PR 4286: remove configure

* PR 4286: more kills

* PR 4286: add --ask 20

* PR 4286: less kills

* PR 4286: skip hook warning

* - more logs

* - even more logs

Signed-off-by: SSE4 <[email protected]>

* - wait for the process

Signed-off-by: SSE4 <[email protected]>

* - it's possible we have several CI jobs which may incorrectly terminate each other with kill pacman

Signed-off-by: SSE4 <[email protected]>

* - guard to pass export on Linux

Signed-off-by: SSE4 <[email protected]>

* - typo

* - ValueError

* - syntax

* - don't raise

* - move downloads to build
- drop version 20161025
- use new conandata.yml format
- avoid unnecessary copying

Co-authored-by: SSE4 <[email protected]>
conan-center-bot pushed a commit that referenced this pull request May 7, 2021
* msys2: do not the library directory propagate through

* msys2: uninstall pkgconf

pkgconf  should be provided by conan

* use tools.get

this is simpler and automatically removes the downloaded file,
making the final package slimmer by 80MB

* clarify the rolling release status

see #4286

* remove arch from conandata

only x86_64 is supported anyway

* kill pacman in case of error

* bump base version

* remove workaround

not needed anymore, now that the base version is up to date

Co-authored-by: Anonymous Maarten <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
8 participants