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

[vcpkg/fortran] Switch to ifort as the Fortran compiler #19413

Closed
wants to merge 61 commits into from

Conversation

Neumann-A
Copy link
Contributor

@Neumann-A Neumann-A commented Aug 6, 2021

(will update versions if this can actually correctly run in CI)

also closes #19983

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 5742e878fc71b4fcb9c019b50f1f887f22db58df -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index 7995aa8..127a8da 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -3001,8 +3001,8 @@
       "port-version": 1
     },
     "lapack-reference": {
-      "baseline": "3.8.0",
-      "port-version": 5
+      "baseline": "3.10.0",
+      "port-version": 0
     },
     "lastools": {
       "baseline": "2020-05-09",
@@ -6612,6 +6612,10 @@
       "baseline": "2021-05-22",
       "port-version": 1
     },
+    "vcpkg-fortran": {
+      "baseline": "1",
+      "port-version": 0
+    },
     "vcpkg-gfortran": {
       "baseline": "3",
       "port-version": 0
diff --git a/versions/l-/lapack-reference.json b/versions/l-/lapack-reference.json
index 96db87c..cf67f47 100644
--- a/versions/l-/lapack-reference.json
+++ b/versions/l-/lapack-reference.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "17d524cc89464b4cdddf4ab9ca3d25b72b6d6ba9",
+      "version-string": "3.10.0",
+      "port-version": 0
+    },
     {
       "git-tree": "1887fc1fcb0c96df1ea24fffc9b045330426e3b6",
       "version-string": "3.8.0",

### BATCH_FILE_PATH
Arguments to pass to the batch file
#]===]
function(z_vcpkg_load_environment_from_batch)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@strega-nil @ras0219-msft : Should I put this into scripts/cmake ? The only reason I did not do it here is because I didn't want to force a world rebuild until the required changes to vcpkg-tool are merged.
I also need this for the community intel triplet. (also to load the required env variables)

@PhoebeHui PhoebeHui added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Aug 9, 2021
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

All manifest files must be formatted

./vcpkg format-manifest ports/*/vcpkg.json

Diff
diff --git a/ports/lapack-reference/vcpkg.json b/ports/lapack-reference/vcpkg.json
index cf79815..c99f3b9 100644
--- a/ports/lapack-reference/vcpkg.json
+++ b/ports/lapack-reference/vcpkg.json
@@ -3,9 +3,7 @@
   "version-string": "3.10.0",
   "description": "LAPACK — Linear Algebra PACKage http://www.netlib.org/lapack/",
   "dependencies": [
-    {
-      "name": "vcpkg-fortran"
-    }
+    "vcpkg-fortran"
   ],
   "default-features": [
     "blas-select"
diff --git a/ports/vcpkg-ifort/vcpkg.json b/ports/vcpkg-ifort/vcpkg.json
index f7c4366..f4525fb 100644
--- a/ports/vcpkg-ifort/vcpkg.json
+++ b/ports/vcpkg-ifort/vcpkg.json
@@ -3,8 +3,7 @@
   "version-string": "1",
   "description": "Metaport to install ifort dll dependencies from Intel oneAPI",
   "supports": "windows & !arm",
-  "dependencies" : 
-  [
+  "dependencies": [
     "vcpkg-fortran"
   ]
 }
After committing all other changes, the version database must be updated
git add -u && git commit
git checkout c801072693edcd0704d187131d9078fcd089dbf2 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index e135641..43969b2 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -3009,8 +3009,8 @@
       "port-version": 1
     },
     "lapack-reference": {
-      "baseline": "3.8.0",
-      "port-version": 5
+      "baseline": "3.10.0",
+      "port-version": 0
     },
     "lastools": {
       "baseline": "2020-05-09",
@@ -6620,10 +6620,18 @@
       "baseline": "2021-05-22",
       "port-version": 1
     },
+    "vcpkg-fortran": {
+      "baseline": "1",
+      "port-version": 0
+    },
     "vcpkg-gfortran": {
       "baseline": "3",
       "port-version": 0
     },
+    "vcpkg-ifort": {
+      "baseline": "1",
+      "port-version": 0
+    },
     "vcpkg-pkgconfig-get-modules": {
       "baseline": "2021-04-02",
       "port-version": 1
diff --git a/versions/l-/lapack-reference.json b/versions/l-/lapack-reference.json
index 96db87c..e090549 100644
--- a/versions/l-/lapack-reference.json
+++ b/versions/l-/lapack-reference.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "d013c13a916469a4a6de4fbbbd164dc3fc607eab",
+      "version-string": "3.10.0",
+      "port-version": 0
+    },
     {
       "git-tree": "1887fc1fcb0c96df1ea24fffc9b045330426e3b6",
       "version-string": "3.8.0",

@Neumann-A
Copy link
Contributor Author

@BillyONeal: VMs not completely setup?

    The license for Visual Studio expires in 1 day.
    Project 'ALL_BUILD' could not be loaded because it's missing install components. To fix this launch Visual Studio installer with the following selections:
    C++ (v142) Universal Windows Platform tools

@BillyONeal
Copy link
Member

@BillyONeal: VMs not completely setup?

Nothing has changed in the VM image in the last month (#19508 isn't landed yet). Note that we install "Universal Windows Platform Tools" but not "Universal Windows Platform Tools (v142)" right now

'Microsoft.VisualStudio.Workload.Universal',

This implies installing the "Universal Windows Platform Development" workload, but I don't see lines for the following optional checkboxes (they are checked on my development workstation but not CI :) ):

image

@Neumann-A
Copy link
Contributor Author

According to https://docs.microsoft.com/en-us/visualstudio/install/workload-component-id-vs-build-tools?view=vs-2019 the missing optional component is Microsoft.VisualStudio.ComponentGroup.UWP.VC.BuildTools

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

Error: Local changes detected for vcpkg-cmake but no changes to version or port version.
-- Version: 2022-04-12
-- Old SHA: ef4a9463f802c1cd6f8b31e560529aeaee0faff4
-- New SHA: 60b5c6f101857827ca5912cf728672f41830d9dc
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***
Error: Local changes detected for vcpkg-fortran but no changes to version or port version.
-- Version: 2022-02-22
-- Old SHA: ec6affed6b4aca0be5d5c59cc5c614f43afbc177
-- New SHA: 06093715131e8d56f325052595c8e1fc8710e936
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***
After committing all other changes, the version database must be updated
git add -u && git commit
git checkout c37cc7836a0e1cfd55747be8ec472eafa8055276 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index fc44c8d7..99f7189c 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -3322,7 +3322,7 @@
     },
     "lapack-reference": {
       "baseline": "3.10.0",
-      "port-version": 0
+      "port-version": 1
     },
     "lastools": {
       "baseline": "2020-05-09",
@@ -7304,6 +7304,10 @@
       "baseline": "2022-02-06",
       "port-version": 0
     },
+    "vcpkg-fortran": {
+      "baseline": "2022-02-22",
+      "port-version": 0
+    },
     "vcpkg-get-python-packages": {
       "baseline": "2022-04-11",
       "port-version": 0
@@ -7316,6 +7320,10 @@
       "baseline": "2021-11-16",
       "port-version": 1
     },
+    "vcpkg-ifort": {
+      "baseline": "2022-02-22",
+      "port-version": 0
+    },
     "vcpkg-pkgconfig-get-modules": {
       "baseline": "2022-02-10",
       "port-version": 0

@JackBoosY
Copy link
Contributor

Will rerun x64-osx tests after others finished.

@Neumann-A Neumann-A marked this pull request as draft April 23, 2022 20:07
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

Error: Local changes detected for lapack-reference but no changes to version or port version.
-- Version: 3.10.0#1
-- Old SHA: 0ce53c448a021788bc0249e2d75b5a6b96e799bb
-- New SHA: d4eae50df1a1b64c8a1ebfb5e2bcca6a77922c1b
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***

@JackBoosY JackBoosY marked this pull request as ready for review April 27, 2022 14:15
@JackBoosY JackBoosY marked this pull request as draft April 27, 2022 14:15
@JackBoosY
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BillyONeal
Copy link
Member

Related: #15053 ?

@Neumann-A
Copy link
Contributor Author

Related: #15053 ?

So so. 15053 switches the whole windows toolchain to intel. 15053 contains stuff from this PR though.

Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

  • Our default behavior has to work without the user installing extra Intel stuff for users who do not care. This probably prevents using ifort by default since it appears to require a separate login-walled installation of Intel's OneAPI toolchains.
  • Capturing a port for the contract of "we need Fortran support", as you have done here with vcpkg-fortran, seems reasonable and consistent with other things that we do. (I'm personally concerned about potential command line differences between them but I'm told that lapack and blas are substantially all our Fortran customers and that I probably shouldn't worry about that)
    • We do think it's reasonable to give users a mechanism to choose a Fortran compiler to use, as you are trying to demonstrate in WIP: [x64-windows-intel] new Triplet: #15053 . A triplet setting, as demonstrated there, would work, but is inefficient because it forces a world rebuild to change this even though a small number of ports may be affected. (Of course, this is true for all triplet settings) Overlays are a solution that affects only things that actually need Fortran, but providing an overlay forces the user to implement the complete interface somehow rather than just saying "I want ifort" or "I want gfortran". Overlays have a nice property in that it lets you do development on the "not default" option without triggering world rebuilds for other customers.
    • We do not think "try to use ifort if we can find it" is OK, it needs to be an explicit user decision, for example, in the triplet.
    • This contract/metaport probably shouldn't have a "supports" clause, unless we know that it's categorically impossible to provide Fortran there. So !uwp may be fine but windows is not. (Linux customers may want to use gfortran from their system's package manager, for example.)
  • [You mentioned this but I'm indicating that we're considering it] This PR breaks x86 hosts (you need to be an amd64 host in order to run this). Some years ago we had a substantial number of x86-only customers. We accidentially broke this ourselves though in [7zip] Upgrade vcpkgTools 7zip to 19.00 msi #19365 and didn't switch back to CMake to extract things for 4 ish months and it seems nobody noticed which suggests that this wouldn't be a big deal.
  • Given that UWP is already broken for gfortran et al. here, don't worry about UWP interaction in whatever the solution is.

I'm sorry if these observations sound a bit "wishy washy" -- the situation is that we aren't 100% sure what we want the future to be here so we've resorted to listing constraints we believe exist on the solution space to help you decide (a) how you should interpret our response as to whether we want this, and (b) whether you want to continue investigating this path.

Suggested path forward:

  1. Add vcpkg-fortran as the representation of the contract, as you have done here. Its behavior should match today's behavior (gfortran).
  2. Document the contract (all the cmake functions, variables, environment variables, etc.) expected to be provided by vcpkg-fortran.
  3. Create a new port intended to be an overlay for vcpkg-fortran which selects ifort. That probably needs to live in a different directory like the hypothetical "backports" we have been talking about, but for purposes of making forward progress on this PR I don't think you need a concrete answer to that question yet.

@BillyONeal BillyONeal added category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed and removed requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist labels Apr 28, 2022
@Neumann-A
Copy link
Contributor Author

Our default behavior has to work without the user installing extra Intel stuff for users who do not care. This probably prevents using ifort by default since it appears to require a separate login-walled installation of Intel's OneAPI toolchains.

With that you are basically saying to take the flang-classic route used in #23987. The main reason I am hesitant about that route is the build times required for the LLVM fork of flang-classic and the fact that #23987 loses x86 target which can still be covered by gfortran. Would be good if that could be reduced somehow

lapack and blas are substantially all our Fortran customers

that is because the Fotran support situation has not been solved and vcpkg is focused around windows. On other platforms Fortran support is easily available. Only windows is a problem.

A triplet setting, as demonstrated there, would work, but is inefficient because it forces a world rebuild to change

It is not a triplet setting but a toolchain setting which sets CMAKE_Fortran_(COMPILER|FLAGS) explicitly. This is only required on windows due to vcpkg cleaning the environment/path. On other platforms CMake will find the fortran compiler without extra help and without needing extra flags.
Getting rid of the world rebuild due to the triplet hash is an open issue #19984.
The only thing which would be nice to have is z_vcpkg_load_environment_from_batch to not have to manually setup the passthrough. Should this be made into a different PR?

rather than just saying "I want ifort" or "I want gfortran". Overlays have a nice property in that it lets you do development on the "not default" option without triggering world rebuilds for other customers.

Personally I would like to use version-string for that in the main repo ;). basically have a PR with version-string : "gfortran"; version-string : "ifort", version-string : "flang-classic", The additional overlays for classic users could also be provided. The question here is what should the end default be in the main repo?
Again this is a windows only issues. All other platforms will control the Fortran compiler via env FC.

This contract/metaport probably shouldn't have a "supports" clause, unless we know that it's categorically impossible to provide Fortran there. So !uwp may be fine but windows is not. (Linux customers may want to use gfortran from their system's package manager, for example.)

All other platforms have native fortran support and don't require the contract/extra port. I don't intend to force the compiler there.

...This PR breaks x86 hosts .... We accidentially broke this ourselves ... nobody noticed which suggests that this wouldn't be a big deal..... Given that UWP is already broken for gfortran et al. here, don't worry about UWP interaction in whatever the solution is.

Ok won't care about x86 host and UWP. This also means I should go with #23987 which additionally adds arm64 support but loses x86 target support. I could make a port vcpkg-fortran depending on nothing (for !windows) and depending on vcpkg-gfortran for x86 or vcpkg-flang-classic for the rest.

the situation is that we aren't 100% sure what we want the future to be here

I personally think that should be clear -> flang-new will be the future path which probably will still require 2 years to mature.

@BillyONeal
Copy link
Member

With that you are basically saying to take the flang-classic route used in #23987. The main reason I am hesitant about that route is the build times required for the LLVM fork of flang-classic and the fact that #23987 loses x86 target which can still be covered by gfortran. Would be good if that could be reduced somehow

Yeah, we wouldn't want to require people build a whole compiler to get BLAS. But clang is available in binary forms today, it seems like it would be reasonable for flang to be similarly available. (It may even involve some amount of hosting by GitHub or the vcpkg team in support of this or...)

Personally I would like to use version-string for that in the main repo ;).

Then we would need to have version versions! It'd be turtles all the way down! I would point to the "alternatives" / "options" RFC as a solution here whenever that finally gets to the top of our backlog.

This contract/metaport probably shouldn't have a "supports" clause, unless we know that it's categorically impossible to provide Fortran there. So !uwp may be fine but windows is not. (Linux customers may want to use gfortran from their system's package manager, for example.)
All other platforms have native fortran support and don't require the contract/extra port. I don't intend to force the compiler there.

The port should still exist to document the dependency even if it becomes empty on the platforms where Fortran compilers are assumed to be "ambiently available".

@Neumann-A
Copy link
Contributor Author

But clang is available in binary forms today, it seems like it would be reasonable for flang to be similarly available.

It is not. Also building the flang1 and flang2 doesn't require that much time. Only building clang with the classig-flang toolchain takes forever (1h on my machine). Due to MT(d) MD(d) stuff there needs to be extra stuff taken into account in the building of clang with flang on windows. I haven't figured out how clang embeds /DEFAULTLIB: into object files.
It probably gets better if flang-new is finally ready to produce binaries maybe it will then be included in the LLVM windows package but currently only ifort/ifx is available in binary form on windows (unless you plan to pull flang from conda.)

Then we would need to have version versions!

that already exists: It is called port-version:

finally gets to the top of our backlog.

Probably when we are all grey and wiser ;)

The port should still exist to document the dependency even if it becomes empty on the platforms where Fortran compilers are assumed to be "ambiently available".

probably can do that and just assume all other platforms use gfortran even though there is ifort and ifx also available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[lapack-reference] build failure - gfortran broken [lapack-reference] update to 3.10.0
6 participants