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

fix: Pin cpu platform #302

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: Pin cpu platform #302

wants to merge 1 commit into from

Conversation

Dorus
Copy link
Collaborator

@Dorus Dorus commented Sep 20, 2024

fixes #299

This sets our builds to the specific platform the dlls we ship.

We could support more platforms, but that would require us to include different dlls. This pr focus just on removing the warning about the architecture missmatch (any cpu) and the dlls we use.

@Dorus Dorus force-pushed the cpu-platform branch 2 times, most recently from d6aa899 to 6ba48d1 Compare September 23, 2024 19:14
@Dorus Dorus marked this pull request as ready for review September 23, 2024 19:15
@Dorus Dorus requested a review from shpaass as a code owner September 23, 2024 19:15
@shpaass
Copy link
Owner

shpaass commented Sep 23, 2024

As none of us are proficient in the sln files, it would be very informative if you explained every change that was done to the files. Essentially, it is the equivalent of me asking "what does it do?" and "why do we need this change?" to every change in the diff.

@Dorus
Copy link
Collaborator Author

Dorus commented Sep 23, 2024

  1. csproj changes: These just tell the project is compatible with this platform option.
  2. .sln changes: This tells visual studio and/or msbuild (the underlying runner of dotnet build/publish) what platforms we support. In our case we support Debug and Release as solution configuration and x64 and ARM64 as platform. There is a bit of aliassing here telling the tools what solution configuration matches with what csproj configuration. All of this is set up with the Configuration Manger in visual studio
    afbeelding
    You could in theory use different platforms and different set of projects to build for each config, but i've just set it to build everything under the same selected platform.
  3. build.sh changes: This tells dotnet to pass paramters to msbuild with the -p flag. The Plaform flag tells msbuild what platform to pick from, since we now want to make sure OSX is build on the correct platform, and not Any CPU that I've removed.

And to why we removed Any CPU: Obvious to get rid of the warning. But more precise, we import DLLs that are only comnpatible with specific platforms, by setting the platform on the project build, we make dotnet check for this platform requirement at startup rather than crash when it tries to load an incompatible dll. Note this wont actually change what's build otherwise, the resulting dotnet code is still platform independent.

@shpaass
Copy link
Owner

shpaass commented Sep 23, 2024

Thank you for the explanation! I guess that the next step of the review would be to figure out why the CI check fails and what we need to do about it.

@Dorus
Copy link
Collaborator Author

Dorus commented Sep 23, 2024

By the look, the CI test run the wrong platform. Let me see if i can update the CI commands.

@Dorus Dorus force-pushed the cpu-platform branch 7 times, most recently from 6a602a2 to b4f5422 Compare September 23, 2024 20:52
@Dorus
Copy link
Collaborator Author

Dorus commented Sep 23, 2024

Yay, that took a few more tries than i would like to admit. First needed to find the right file, then the right command. Anyway i think i got it since the CI check is now green. I've rebased and squashed all changes into one commit.

fixes shpaass#299

Move from any cpu to specific platforms since we use dlls that do not
support any cpu.
Set build script to specific platforms.
Set github action: dotnet test to x64 specifically since arm64 wont run on github.
@shpaass
Copy link
Owner

shpaass commented Sep 28, 2024

I might be wrong, but the criterion of #299 being fixed is that when you run dotnet format, there's no line Warnings were encountered [...].

This line is still present with the current PR.

@Dorus
Copy link
Collaborator Author

Dorus commented Sep 28, 2024

This should fix the

There was a mismatch between the processor architecture of the project being built

error. It was present for me when i started working on this, and gone after these changes. However right now i do not get that error anymore at all, both on master or my branch. I'm also not getting any other errors from dotnet format, both locally and from the git hub build.

@shpaass
Copy link
Owner

shpaass commented Sep 28, 2024

That's curious because I still get this error on your branch.

@skullbearer
Copy link

skullbearer commented Oct 5, 2024

yafc20241005.log

I receive no warnings or errors. Logfile attached. The cpu-platform branch build runs fine, no noticeable change in load or operating time vs main branch.

@shpaass
Copy link
Owner

shpaass commented Oct 5, 2024

@skullbearer
To make sure,
You get the warning when Yafc doesn't have the PR patch.
You get no warning when Yafc has the PR patch.
Is that correct?

@skullbearer
Copy link

skullbearer commented Oct 8, 2024 via email

@Dorus
Copy link
Collaborator Author

Dorus commented Oct 8, 2024

Is anyone at this point able to reproduce the error in #299 on windows? And if so, does it go away when switching to this branch (and does it return when you switch back to master?)

Anything that helps me reproduce this on windows would help me, even if it's just compiling from the command line or git bash.

@shpaass
Copy link
Owner

shpaass commented Oct 8, 2024

The command that I used for today's test is

dotnet format --verify-no-changes --diagnostics IDE0055 --severity info --verbosity minimal

and I went by checking if it says Warnings were encountered.

The branches where the warning appeared:
shpaass:master
shpaass:linked-item-in-problematic-row
Dorus:cpu-platform

The warning did not appear on Dorus:master both before and after fixing what it detected.

I'm doing a bisection now.

@shpaass
Copy link
Owner

shpaass commented Oct 8, 2024

I confirmed that the commit ea3211a indeed does not trigger a warning in shpaass:master.
The bisection identified 91595b5 as the commit that introduced the warning. In other words, it indeed is #191

@skullbearer
Copy link

It would seem then the issue is no longer extant?

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.

Processor architecture mismatch. MSIL and AMD64
3 participants