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

Compile libsass for one platform at a time #53

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

am11
Copy link
Contributor

@am11 am11 commented Jan 19, 2017

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.

Fixes #47.

@am11
Copy link
Contributor Author

am11 commented Jan 19, 2017

/cc @nschonni

Copy link
Contributor

@nschonni nschonni left a 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;
Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

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

Copy link
Contributor Author

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.
@am11 am11 force-pushed the feature/single-paltform-build branch from 68861f8 to 55fa474 Compare January 19, 2017 18:14
@am11
Copy link
Contributor Author

am11 commented Jan 19, 2017

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.

Copy link
Contributor

@nschonni nschonni left a comment

Choose a reason for hiding this comment

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

<!-- Windows -->
<MSBuild Projects="..\LibSass\win\libsass.sln" BuildInParallel="true"
Condition="'$(LibSassPlatform)' == 'Win32' or '$(LibSassPlatform)' == 'Win64'"
Properties="Configuration=$(Configuration);Platform=$(LibSassPlatform);OutDir=$(OutputPath);IntDir=$(IntermediateOutputPath)" />
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

@am11 am11 Jan 19, 2017

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.

@am11
Copy link
Contributor Author

am11 commented Jan 19, 2017

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)

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.

2 participants