-
Notifications
You must be signed in to change notification settings - Fork 32
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
Compile libsass for one platform at a time #53
base: master
Are you sure you want to change the base?
Conversation
f430172
to
68861f8
Compare
/cc @nschonni |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the separate DLLs are necessary for the NuGet packaging (see the nuspec)
using static System.IO.File; | ||
using static System.IO.Path; | ||
using static LibSass.Tests.TestCommonsAndExtensions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These can probably be undone. Nothing wrong with the sort, just not related to the changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, I ran batch formatter at the end of changes and this one was odd one out. All the rest of the code files in entire repo have usings sorted by same rule. We can do these changes separately if you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't really worry about it, but I'd usually just throw it in another commit for clarity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, this change is now part of a separate commit.
Added an inline MSBuild task, which make use of a new CoreFX API `System.Runtime.InteropServices.RuntimeInformation` to obtain OS description and architecture information, then translates into `LibSassPlatform` (Win32 or Win64 etc.). Also added ability to cross compile, e.g. Win32 on 64-bit system. Additional OSes will be added with .NET Core support. Added cross compile configurations for AppVeyor CI.
68861f8
to
55fa474
Compare
This change is good to go. The non-reflection version of task needs MSBuild15, which we will migrate to eventually for .NET Core support but that is for a later day. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pack https://github.com/sass/libsass-net/blob/master/pack.cmd and Nuspec https://github.com/sass/libsass-net/blob/master/LibSass.NET/LibSass.Net.nuspec will need to be updated if this is the way to go
<!-- Windows --> | ||
<MSBuild Projects="..\LibSass\win\libsass.sln" BuildInParallel="true" | ||
Condition="'$(LibSassPlatform)' == 'Win32' or '$(LibSassPlatform)' == 'Win64'" | ||
Properties="Configuration=$(Configuration);Platform=$(LibSassPlatform);OutDir=$(OutputPath);IntDir=$(IntermediateOutputPath)" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The layout needs to take into consideration that multiple platforms are building into the same folder. Take a look at https://github.com/sass/libsass-net/pull/52/files#diff-87bd0df6df8e00153597365c4585e239R92 for an idea on putting it at least under a "platform" folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please read the description of this PR and the associated issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good because one at a time make sense, but this breaks NuGet packaging , or just clobbers the folder in the case you want to build 32bit on a 64 bit OS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
From slack:
yup the packaging story would be like, for windows, we will run a script to build on x64 machine which will build for all four architectures (x86,x64,arm,arm64) one at a time and place into separate directories for packing. We will compile linux and osx binaries separately before hand and place in the same build machine, the pack script will look something like
pack --linux-bins=path\to\linux\bins --osx-bins=path\to\osx\bins --win-bins=path\to\win\bins
or--win-bins=compile
.
or to avoid doing it manually in a batch file, we can also create our custom dotnet pack command which can extend dotnet-pack: say dotnet-sassPack
. That way we would be able to pack on Unix systems as well without writing a separate .sh counterpart.
This is the whole point of this exercise to build for single platform with unified name. The DLL search path need the file in same directory. The P/Invoke requires a constant name of the DLL in the DLL search patch, which cannot be nested. Packaging step is a separate work item, where we bring dlls from different sources (Linux, OSX boxes and/or cross-compile) |
Added an inline MSBuild task, which make use of a new CoreFX API
System.Runtime.InteropServices.RuntimeInformation
to obtain OSdescription and architecture information, then translates into
LibSassPlatform
(Win32 or Win64 etc.).Also added ability to cross compile, e.g. Win32 on 64-bit system.
Additional OSes will be added with .NET Core support.
Added cross compile configurations for AppVeyor CI.
Fixes #47.