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

Portable build tool / Windows testing #1026

Merged
merged 45 commits into from
Nov 3, 2023
Merged

Portable build tool / Windows testing #1026

merged 45 commits into from
Nov 3, 2023

Conversation

dabrahams
Copy link
Collaborator

@dabrahams dabrahams commented Sep 24, 2023

Fixes #805

@@ -4,6 +4,7 @@ import Utils

/// A command-line tool that generates XCTest cases for a list of annotated ".hylo"
/// files as part of our build process.
@main
Copy link
Contributor

Choose a reason for hiding this comment

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

Last time this change got merged it caused Windows to fail. That had been done because we couldn't write GenerateHyloFileTests.main() at the top level. My fix was to create the main.swift file that this PR wants to remove. Is that change necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it's necessary. Use of @main is fine on all platforms if you pass the (required on all platforms but not enforced) --parse-as-library option to swift, as I have done in Package.swift.

@dabrahams dabrahams force-pushed the portable-build-tool branch from 8d58528 to 7b3f6b2 Compare October 10, 2023 23:13
@dabrahams
Copy link
Collaborator Author

@C-BJ I have pushed this a long way down the road to working; in fact, the tests all run on my local Windows VM. Most of them, by far, pass (there are some problems with the executables we generate). I need to focus on other things and don't really have the tools to diagnose the problem myself, but would be happy to work with you to push it over the line. Then we would be in a good position to try to address #864. WDYT?

@dabrahams
Copy link
Collaborator Author

dabrahams commented Oct 11, 2023

Regardless, the current status of this PR is:

  • the tests all run on my local Windows VM. Most of them, by far, pass

  • there are some undiagnosed problems with the executables we generate on Windows, causing some tests to fail.

  • in CI where PortableBuildToolPlugin's invocation of a swift run GenerateHyloFileTests... command fails to find the executable after building it due to an SPM path bug. This works fine on my Windows VM so I'm hoping someone else can reproduce it locally.

  • My Windows VM is an ARM64, and the GitHub runners are Intel; it would eliminate some variables to diagnose this on an Intel machine.

  • Once we get things working, there are few things we should try to roll back: c0c78cd added --verbose flags that shouldn't be needed.

  • The Swift installation on Windows is currently a prerelease from the Browser Company; the official build probably works (see build-and-test.yml from 36fabb3)

@dabrahams
Copy link
Collaborator Author

dabrahams commented Oct 11, 2023

One way to get this running in CI is to remove the ArgumentParser dependency from GenerateHyloFileTests and invoke it as swift Sources/GenerateHyloFileTests/GenerateHyloFileTests.swift ... instead of swift run GenerateHyloFileTests ...

That will work around the SPM path bug by not using SPM.

I'd be happy to make that update.

This is really a flawed approach.  We need a FileSystemPath type.
@C-BJ
Copy link
Contributor

C-BJ commented Oct 15, 2023

@C-BJ I suspect the reasons we are generating broken executables on Windows has something to do with the way we are linking and/or building the support library. A few questions come to mind:

  • Why are we building the support library with clang rather than with MSVC?
  • Are we sure lld-link is the right tool to be using to link here? If you notice, Linux executables need to be linked by invoking clang so that its C library is available to them. Is it possible we need to link using MSVC?
  1. I don't think it matters either way.
  2. Now lld-link has links to the MSVC runtime, and I think it's fine to choose either MSVC link or lld-link as well

@C-BJ
Copy link
Contributor

C-BJ commented Oct 15, 2023

@dabrahams I think we should put the support library in the same folder as hc so we can link it easily. I'm trying to figure out how to get the path to hc.exe, that is, to have Swift tell us where the current program execution is located. But I'm not sure if the test will pass properly?

@C-BJ
Copy link
Contributor

C-BJ commented Oct 15, 2023

@C-BJ
Copy link
Contributor

C-BJ commented Oct 15, 2023

As far as I know, we are using debug mode build for Windows on CI right now, but the CI title says release.
Do we need to have both modes tested?

@dabrahams
Copy link
Collaborator Author

dabrahams commented Oct 15, 2023

@dabrahams I think we should put the support library in the same folder as hc so we can link it easily.

Please don't worry about where we put this library until you figure out how to make the executables generated by hc pass the tests. Right now CI is putting the support library in the working directory from which hc is invoked, which seems to satisfy the linker. Once we understand what it takes to make working executables we can make a proper target for the support library and figure out how to get it copied into the resource bundle of hc.

As far as I know, we are using debug mode build for Windows on CI right now, but the CI title says release.
Do we need to have both modes tested?

No, we should update the title. We shouldn't worry about testing release builds until we have gotten debug builds to generate working executables.

CI is still flaky: https://github.com/hylo-lang/hylo/actions/runs/6520168166/job/17707473362?pr=1026

CI isn't flaky; it's just that I tried to do this. I believe we need to be on a release snapshot rather than the final release; I'll make that change… but there is no Windows release snapshot there, so we need to go back to the Browser Company build of Swift. Done.

@dabrahams
Copy link
Collaborator Author

  1. I don't think it matters either way.
  2. Now lld-link has links to the MSVC runtime, and I think it's fine to choose either MSVC link or lld-link as well

Do you have another theory about what makes the generated executables fail to work? If not, maybe it's worth trying these other configurations to see if they make a difference.

@C-BJ
Copy link
Contributor

C-BJ commented Oct 15, 2023

Do you say the test, or using command.
AFAIK, We're not linking the support library correctly.

@dabrahams dabrahams force-pushed the portable-build-tool branch from 45d413a to 6be1064 Compare October 15, 2023 03:02
@dabrahams
Copy link
Collaborator Author

Do you say the test, or using command.

Sorry, I don't understand the statement (or question, if that's what it is).

@C-BJ
Copy link
Contributor

C-BJ commented Oct 15, 2023

I'll try to fix it, but I don't have much time.
btw, How do I get the system path of my currently running program in Swift?

@C-BJ
Copy link
Contributor

C-BJ commented Oct 15, 2023

Maybe let kyouko-taiga see if there's any relationship between those failed tests?

@dabrahams
Copy link
Collaborator Author

How do I get the system path of my currently running program in Swift?

I don't know, sorry. I also don't think you should be trying to put the library where hc.exe is, or into your MSVC installation for that matter. Linkers accept full paths to libraries and also library search path entries on their command lines. If you need to point at the support library, you can use that facility. Unless the link command somehow fails to report an error when you remove or rename the support library, the library is being found by the linker. And I can confirm that hc fails to generate an executable when the support library is missing.

@dabrahams
Copy link
Collaborator Author

Maybe let kyouko-taiga see if there's any relationship between those failed tests?

Feel free to ask her. I have tried to find a relationship and come up empty.

@dabrahams
Copy link
Collaborator Author

@compnerd suggests that we need to use clang to link on Windows, like on linux. I havent' had time to investigate.

@dabrahams
Copy link
Collaborator Author

Update: this problem is solved, I think:

  • Currently on Windows you need to swift build before swift test or swift build --build-tests, because the test generator build plugin is calling swift run --skip-build .... I'm confident there's a workaround for this shortcoming, though it will probably increase your build times.

If you don't want to increase build times and are willing to live with the inconvenience of the above dance, you can set HYLO_NO_REENTRANT_BUILD=anything in the environment.

@dabrahams
Copy link
Collaborator Author

Diagnostics were added that reveal it is crashing in LLVM or Swifty-LLVM doing the mandatory passes:

swift run --skip-build --pkg-config-path . hc --verbose --emit llvm Tests\EndToEndTests\TestCases\AddressOf.hylo && echo YES
begin depolymorphization pass.
create LLVM target machine.
create LLVM program.
LLVM mandatory passes.

C:\Users\dave\src\hylo>

@dabrahams dabrahams merged commit 230560c into main Nov 3, 2023
10 checks passed
@dabrahams dabrahams deleted the portable-build-tool branch November 3, 2023 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable testing on Windows
3 participants