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 msvc compilation #132

Merged
merged 12 commits into from
Sep 9, 2024
Merged

Conversation

harrisi
Copy link
Contributor

@harrisi harrisi commented Aug 28, 2024

This is my (mostly) minimal set of changes to get Windows compilation working. There's still a couple things I want to add to this (cross-architecture compilation, maybe ability to pass different options to vswhere, etc.), but this is enough, I think, for one to compile a simple Bundlex app on Windows for Windows.

One thing of note is the use of vswhere itself. As far as I understand, it's been the preferred way to find Visual Studio-related locations for various things since Visual Studio 2017, so I think it's within the realm of general availability. I could add back the option for an environment variable, but I believe the intent for Windows development is to modify the paths in a way where vswhere can still find them? I haven't done any Windows development since before VS 2017 came out, so I'm not sure. If there are any Windows experts that can chime in, that would help.

I still am not thrilled with this solution, as the build script is
harder to read.
lib/bundlex/toolchain/visual_studio.ex Outdated Show resolved Hide resolved
harrisi and others added 3 commits August 30, 2024 01:17
@harrisi
Copy link
Contributor Author

harrisi commented Aug 30, 2024

The last thing I'm working on which will get this into a place that seems reasonable (after review, of course) is just getting tests working.

@harrisi
Copy link
Contributor Author

harrisi commented Aug 31, 2024

I'd like some feedback on 6482bb8 and f6e6740. As far as I understand, these are the possible values, which don't really line up with the typical cpu values. I'd also rather have Erlang return a proper triple - I opened an issue, but previous and current OTP builds still need this sort-of-hacky handling, I think.

If there's any recommendation for how to handle this better, I'm all ears (or eyes, I guess).

EDIT: I'm realizing that the architecture is used in the test suite for some precompiled ffmpeg binaries. I'm not sure if it's used specifically in any plugins that would need updating, but something to consider.

lib/bundlex.ex Outdated Show resolved Hide resolved
lib/bundlex.ex Outdated Show resolved Hide resolved
lib/bundlex.ex Outdated Show resolved Hide resolved
@mat-hek
Copy link
Member

mat-hek commented Sep 2, 2024

I'd also rather have Erlang return a proper triple - I opened erlang/otp#8767, but previous and current OTP builds still need this sort-of-hacky handling, I think.

If there's any recommendation for how to handle this better, I'm all ears (or eyes, I guess).

I'd love to help, but I have no idea 😕

EDIT: I'm realizing that the architecture is used in the test suite for some precompiled ffmpeg binaries. I'm not sure if it's used specifically in any plugins that would need updating, but something to consider.

Yes, we ship precompiled binaries and they are used in various plugins, but, of course, not in Windows, so this shouldn't break anything, as behaviour on Unix is unchanged

@harrisi
Copy link
Contributor Author

harrisi commented Sep 2, 2024

I'd love to help, but I have no idea 😕

There's an ongoing PR related to Windows builds on arm64, which is apparently the main active work touching things like host triples from :erlang.system_info(:system_architecture). I haven't dug into it yet to see if I'm capable (or willing) to spend much effort here, but I'll see. I think for the needs of Bundlex it's not that important, and this sort of awkward special handling will be necessary for awhile. At some point it can be removed, but that'll probably be years from now.

Yes, we ship precompiled binaries and they are used in various plugins, but, of course, not in Windows, so this shouldn't break anything, as behaviour on Unix is unchanged

Sorry, I wasn't very clear (or accurate). I was more thinking about if any plugins match on architecture. The values of %PROCESSOR_ARCHITECTURE% are different than normal values (which I think are from GNU's gettext?). On my machine, for example, the value is AMD64. I believe the others are IA64 and ARM64 for 64-bit, and X86 for 32-bit. Note these are different from the link, which is for

I could canonicalize these to what's currently seen from Unix/Darwin values, but I'm not sure if it's worth it, especially since OTP might use different values in the future. So I think just using the output of %PROCESSOR_ARCHITECTURE% is the safest bet for now, and anyone handling Windows-specific code just needs to know about that difference.

@mat-hek
Copy link
Member

mat-hek commented Sep 3, 2024

I was more thinking about if any plugins match on architecture.

Nope, they match on the OS too, I don't think we'll have anything specific to architecture but generic to OS ;)

Please format the code and we should be good to merge 🚀

@harrisi
Copy link
Contributor Author

harrisi commented Sep 4, 2024

I think I got cnodes and ports working correctly, ish. Can't fully test yet because of other issues (test suite relies on pkg-config, trying to build mad mp3 plugin has issues from shmex, etc.), but so far it's looking promising. I think this is at the point of functionally being able to be merged.

@mat-hek mat-hek merged commit 4c2a60c into membraneframework:master Sep 9, 2024
3 checks passed
@harrisi harrisi deleted the fix-windows-vs branch September 9, 2024 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants