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

[intel-mkl] Overhaul, install osx #30483

Merged
merged 18 commits into from
May 11, 2023
Merged

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Mar 28, 2023

Uses hdiutil to attach/detach the dmg. I'm unsure if we should try hard to always detach to avoid CI problems.

Intentionally putting osx libs into lib/intel64 as on linux because it plays well with cmake BLAS find/ignore setup.

Update: #30483 (comment)

  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@dg0yt
Copy link
Contributor Author

dg0yt commented Mar 28, 2023

Looks okay with arrayfire and coin-or-ipopt building successfully.
CC @Neumann-A

@dg0yt dg0yt marked this pull request as ready for review March 28, 2023 07:40
@Neumann-A
Copy link
Contributor

Looks okay with arrayfire and coin-or-ipopt building successfully.

Hmm looks like I didn't adjust coin-or-ipopt to use --with-intsize=64 after the change from lp64 to ilp64. My overlay does that but the current port in vcpkg doesn't do that.

@Cheney-W Cheney-W added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Mar 28, 2023
Cheney-W
Cheney-W previously approved these changes Mar 28, 2023
@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label Mar 28, 2023
@vicroms vicroms added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Mar 28, 2023
@vicroms
Copy link
Member

vicroms commented Apr 6, 2023

@dg0yt is it possible to extract the dmg file using 7zip?

@Neumann-A
Copy link
Contributor

@dg0yt is it possible to extract the dmg file using 7zip?

Even though I wasn't asked I already established in the original PR that it is possible. The problem however is to bring the posix variant of of 7zip into vcpkg since the team still has not decided on how to bring tools into vcpkg.

@vicroms
Copy link
Member

vicroms commented Apr 6, 2023

Personally, I would add it to find_acquire_program(7z)

@Neumann-A
Copy link
Contributor

Personally, I would add it to find_acquire_program(7z)

I am against that due to abi_info:
vcpkg_find_acquire_program fdd8d8e0bf5217df8f40239e826c6f37c0c052ee2024c265a5916e8f7eef4107

changing vcpkg_find_acquire_program basically triggers a world rebuild. I would prefer a solution which acts like vcpkg_find_acquire_program but only has a cone of destruction which covers the ports using 7z. tool ports with a vcpkg-port-config.cmake could do that.

@BillyONeal
Copy link
Member

I would prefer a solution which acts like vcpkg_find_acquire_program but only has a cone of destruction which covers the ports using 7z. tool ports with a vcpkg-port-config.cmake could do that.

:sigh: I'm working on it:

spec screenshot

fetch mechanism desires table

tool list

I think the medium term solution for compressors and decompressors will be via vcpkg fetch/vcpkg_find_acquire_program because

  • they are likely to be on the system
  • they are likely to be relatively version agnostic
  • they are involved in 'baseline' activities we must do before we can run a portfile.cmake, like binary caching and bootstrapping other tools like cmake
  • it makes sense that host systems might be opinionated about particular implementation; for example an embedded platform where the user compiled a special build for their particular CPU

I'm shopping this around with other maintainers this week.

@BillyONeal
Copy link
Member

We didn't get a chance to talk about the tool ports in today's maintainer meeting, ran out of time. Will probably get a go/no go on my concrete spec proposal on Thursday.

My current WIP proposal:

The Future of Tools in vcpkg.docx
The Future of Tools in vcpkg - Supporting Workbook.xlsx

TL;DR: Unless there's a reason to do otherwise, => tool ports. Reasons to do otherwise:

  • Must be available before invoking cmake
  • Compressor/decompressor
  • Binary caching provider

Under these rules most of the things @Neumann-A has tried to submit as tool ports qualify as OK but not vcpkg-tool-7zip. But I have not gotten feedback from other maintainers yet.

@BillyONeal BillyONeal added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Apr 12, 2023
@BillyONeal BillyONeal self-assigned this Apr 12, 2023
@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 12, 2023

I would continue with adding 7z for osx when #30780 finds support.

@Neumann-A
Copy link
Contributor

My current WIP proposal:

You don't expect us to have access to that, right?

@BillyONeal
Copy link
Member

You don't expect us to have access to that, right?

They should be just attached docs? If you can't open word docs with libreoffice or similar I don't know if I can do much... our bosses and boss like entities run on Office

@BillyONeal
Copy link
Member

Let me try again:

Future of Tools.pdf

image

@Neumann-A
Copy link
Contributor

My comments; TL;DR: just small comments; It all depends on how you are going to spec vcpkg-tool-xxx Future.of.Tools.pdf
I think a lot of no's in the vcpkg-tool-xxx column actually depends on the implementation of the tool as a port.

Also pkgconfig is a fetch tool because of vcpkg_find_acquire_program? I think that could easily be a tool port.

@Cheney-W Cheney-W removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Apr 21, 2023
@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 22, 2023

@dg0yt is it possible to extract the dmg file using 7zip?

The answer is no ATM.

@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 27, 2023

@dg0yt is it possible to extract the dmg file using 7zip?

The answer is no ATM.

I looked at the 7zip source code: Both DMG and APFS source file start with #include <StdAfx.h>. I take this as: Only supported on Windows.

This leaves two options for intel-mkl:x64-osx

  • Use hdiutil (with safety guards)
  • Take the files from the python packages instead.

@dg0yt dg0yt changed the title [intel-mkl] Install osx [intel-mkl] Overhaul, install osx Apr 30, 2023
@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 30, 2023

This has become a little bit more than just osx install:

  • Removed non-working code for unsupported triplets.
  • Changed variable names.
  • Moved more common steps out of the OS specific steps.
  • Uniformly install the same license files.
  • Always install the usage file.
  • Install the regular pkgconfig file for the selected config.

W.r.t. to osx hdiutil:

  • 7zip is not ready as an alternative.
    • Upstream has no binary with DMG and APFS support for osx ATM.
    • The vcpkg port has a poor vendored build system, cannot build the tool ATM.
  • There is now a separate cmake script to do the mount, to quickly copy the archives, and to unmount, proceeding to unmount even on errors (but not when interrupted).
  • There are now message to mark the step which should not be interrupted.

If there is still reluctance to accept this into the repo, we should either disable it temporarily and/or add an evironment variable to enable the use of hdiutil.

@dg0yt dg0yt marked this pull request as ready for review April 30, 2023 19:46
Cheney-W
Cheney-W previously approved these changes May 4, 2023
@Cheney-W Cheney-W added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels May 4, 2023
@JavierMatosD
Copy link
Contributor

Hi @dg0yt,

I have a couple of questions regarding the hdiutil script.

  1. Would the script work for an unprivileged user?
  2. If something interrupts the script (runs out of disk space, etc.), what state is the user left in? For example, would the user be able to delete their vcpkg instance? Would it unmount on reboot?
  3. Can the mount point happen in a temp directory?

Note: I will be converting your PR to draft status. When you respond, please revert to "ready for review". That way, I can be aware that you've responded since you can't modify the tags.

@JavierMatosD JavierMatosD marked this pull request as draft May 4, 2023 21:34
@dg0yt
Copy link
Contributor Author

dg0yt commented May 5, 2023

  1. Would the script work for an unprivileged user?

Yes. This is the only mode I test. (To be clear: A user that can use xcode.)

  1. If something interrupts the script (runs out of disk space, etc.), what state is the user left in? For example, would the user be able to delete their vcpkg instance? Would it unmount on reboot?

If something interrups the script after attaching, the image will remain attached.

Note that this must be a forceful interruption. Running out of disk space during copying isn't enough. Unmounting will also be attempted if copying fails. This is the whole point of using execute_process for the copying instead of doing it directly in CMake.

The user can detach the image with hdiutil or with a single click in Finder (the counterpart of Windows Explorer).

I verified that it is not remounted on reboot.

  1. Can the mount point happen in a temp directory?

Yes. I verified that I can mount to a directory created with mktemp -d.

@dg0yt dg0yt marked this pull request as ready for review May 5, 2023 06:53
@JavierMatosD
Copy link
Contributor

Yes. I verified that I can mount to a directory created with mktemp -d.

From a concurrency safety standpoint, would it make sense to let the mount point be a unique temp directory?

@dg0yt
Copy link
Contributor Author

dg0yt commented May 5, 2023

Yes. I verified that I can mount to a directory created with mktemp -d.

From a concurrency safety standpoint, would it make sense to let the mount point be a unique temp directory?

I don't know but there is a reason why I made the test with mktemp -d. I can change it to use that approach if desired. Then the ownership is clearly contained in the small script.

@JavierMatosD
Copy link
Contributor

Yes. I verified that I can mount to a directory created with mktemp -d.

From a concurrency safety standpoint, would it make sense to let the mount point be a unique temp directory?

I don't know but there is a reason why I made the test with mktemp -d. I can change it to use that approach if desired. Then the ownership is clearly contained in the small script.

Ok, the PR looks good to me, pending the change to a unique temp directory for the mount point. Moving the PR to draft. Please set it to "ready for review" once changes have been applied.

@JavierMatosD JavierMatosD marked this pull request as draft May 10, 2023 19:52
@dg0yt dg0yt marked this pull request as ready for review May 11, 2023 06:08
@JavierMatosD JavierMatosD merged commit ed8fb07 into microsoft:master May 11, 2023
@dg0yt dg0yt deleted the intel-mkl branch May 11, 2023 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants