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

Update maintainer kb windows doc #1956

Merged
merged 14 commits into from
Jun 27, 2023

Conversation

mfisher87
Copy link
Member

@mfisher87 mfisher87 commented May 10, 2023

PR Checklist:

  • note any issues closed by this PR with closing keywords
  • put any other relevant information below

I went through the process of testing on Windows today, and as a non-Windows user I felt these changes would have helped me! A little bit of information was out-of-date.

@mfisher87 mfisher87 requested a review from a team as a code owner May 10, 2023 02:05
@mfisher87
Copy link
Member Author

mfisher87 commented May 11, 2023

/home/runner/work/conda-forge.github.io/conda-forge.github.io/src/maintainer/knowledge_base.rst:: ERROR: Anonymous hyperlink mismatch: 2 references but 0 targets.

Not sure right now what's causing this! A line number would be nice. 🤔

EDIT: missing space before < in hyperlinks was the cause.

mfisher87 and others added 3 commits May 11, 2023 12:29
Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>
Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>
@mfisher87 mfisher87 requested a review from jaimergp May 11, 2023 18:43
Copy link
Member

@jaimergp jaimergp left a comment

Choose a reason for hiding this comment

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

LGTM!

I'll leave it open for the weekend in case some else from @conda-forge/core wants to add something (e.g. wrt the Python 3.11 / Visual C++ version topic). Maybe @h-vetinari knows something too? 🙏

Otherwise I'll merge on Monday!

Copy link
Member

@jaimergp jaimergp left a comment

Choose a reason for hiding this comment

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

LGTM!

I'll leave it open for the weekend in case some else from @conda-forge/core wants to add something (e.g. wrt the Python 3.11 / Visual C++ version topic). Maybe @h-vetinari knows something too? 🙏

Otherwise I'll merge on Monday!

Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

Maybe @h-vetinari knows something too? 🙏

Thanks for the ping, left some small comments/observations.

src/maintainer/knowledge_base.rst Outdated Show resolved Hide resolved
src/maintainer/knowledge_base.rst Outdated Show resolved Hide resolved
mfisher87 and others added 2 commits May 12, 2023 19:04
…-windows-doc

Naively merge with my changes as best I can!

Co-authored-by: Wolf Vollprecht <w.vollprecht@gmail.com>
Co-authored-by: Matt Fisher <mfisher87@gmail.com>
@mfisher87 mfisher87 force-pushed the update-maintainer-kb-windows-doc branch from e99d98c to 686ea8f Compare May 13, 2023 01:23
@jaimergp
Copy link
Member

Thanks for the updates @mfisher87! A couple more comments and we are good to go from my side!

mfisher87 and others added 2 commits June 22, 2023 09:51
Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>
Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>
@mfisher87 mfisher87 requested a review from jaimergp June 22, 2023 15:53
@mfisher87
Copy link
Member Author

mfisher87 commented Jun 22, 2023

Thanks @jaimergp! I think I got everything :)

I'm a little confused though about this link you shared. Should these docs show an example of what arm64 support should look like as well? e.g.:

  - vs2019                     # [win and x86_64]
  - vs2022                     # [win and arm64]

I don't really understand why we'd want to compile with vs2019 for x86. Why not vs2022 for both arm64 and x86_64 platforms? I know very little about the Windows world :)

@jaimergp
Copy link
Member

Should these docs show an example of what arm64 support should look like as well? e.g.:

  - vs2019                     # [win and x86_64]
  - vs2022                     # [win and arm64]

I don't really understand why we'd want to compile with vs2019 for x86. Why not vs2022 for both arm64 and x86_64 platforms? I know very little about the Windows world :)

I don't know much either 😬 Maybe conda-forge will upgrade to v2022 generally in the future, but for now that's the status quo and that's what we should document.

Support for arm64 is still being worked on so I don't think we need to cover it yet (specially because there are no other options available; you just get vs2022, technically speaking.).

@mfisher87
Copy link
Member Author

Maybe conda-forge will upgrade to v2022 generally in the future, but for now that's the status quo and that's what we should document.

I'm even more confused :) I'm having a slow-brain morning I guess! How does that affect this PR which is changing the docs from recommending vs2019 to vs2022?

@h-vetinari
Copy link
Member

I don't really understand why we'd want to compile with vs2019 for x86. Why not vs2022 for both arm64 and x86_64 platforms? I know very little about the Windows world :)

I don't know much either 😬 Maybe conda-forge will upgrade to v2022 generally in the future, but for now that's the status quo and that's what we should document.

On windows, the compilers come as an integrated toolchain (not separate components like on unix). Updating the toolchain requires everyone who wants to compile against conda-forge windows artefacts to also upgrade their toolchain. It's a bit hard to quantify because it doesn't affect users just downloading conda packages, but we've been conservative about this (FWIW, I wanted to upgrade faster from vs2017 to vs2019, but there was opposition. Eventually it lead to #1732, which explains the situation more in-depth).

That's the baseline. However, win-arm64 is (effectively) a brand-new platform, so there's no-one relying on conda-forge win-arm64 artefacts of any kind yet, and we need the newer toolchain because arm64-support is still being ramped up (and vs2019 likely not good enough).

So now we have to split the toolchain version per architecture, until such time when all of conda-forge moves to vs2022, then they can move in sync from then on.

I left a change request related to this.

@mfisher87 mfisher87 requested a review from jaimergp June 23, 2023 03:52
@jaimergp
Copy link
Member

Thanks @h-vetinari! I think this is good to go now then!

Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

There's still some heavily outdated stuff in the "Building for different VC versions" section, but we can tackle that in a separate PR IMO, because it might need more work/discussion, and I don't want to hold up this PR.

Thanks for the PR & the patience!

@mfisher87
Copy link
Member Author

Thanks for the PR & the patience!

Any time, thanks for being welcoming :)

@jaimergp jaimergp merged commit 4c83afd into conda-forge:main Jun 27, 2023
1 check passed
@mfisher87 mfisher87 deleted the update-maintainer-kb-windows-doc branch June 27, 2023 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants