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

Update VS build facade #2288

Merged
merged 1 commit into from
Jan 30, 2017
Merged

Update VS build facade #2288

merged 1 commit into from
Jan 30, 2017

Conversation

am11
Copy link
Contributor

@am11 am11 commented Jan 6, 2017

@xzyfer, @mgreter,

I am not well familiar with the syntax of resource files. Just hacked it using Visual Studio's RC editor and updated only few parts (note that I manually updated the .rc file using sublime as editing + saving with VS results in multiple files noise such as .db, .aps and whatnot..)

Added a .SolutionItems folder to VS solution with some repo files including the resource.rc, so it gets visible in Solution Explorer of VS.

Added .db and .aps to .gitignore.

VALUE "OriginalFilename", "libsass.dll"
VALUE "ProductName", "Libsass Library"
VALUE "ProductVersion", "0.9.0.0"
VALUE "ProductName", "LibSass Library"
Copy link
Contributor

Choose a reason for hiding this comment

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

This has bugged me since I discovered this file :)

@xzyfer
Copy link
Contributor

xzyfer commented Jan 6, 2017

@am11 you're my hero 🎉

Are there any good resource on the recommended versioning schema for Windows, or is it fine to just mirror our libtool versioning?

@am11
Copy link
Contributor Author

am11 commented Jan 6, 2017

@xzyfer, I just mirrored the version numbers. As far as I can tell, the version conventions are not set in stone and there is quite a lot of variety of versions in win32 libs. For instance, instead of four-sequence identifier, we can even use three sequences (semver style).

The RC file can read from other .h defines. I was thinking, if libtools and resource.rc share common .h file, that would make our life slightly easier. :)

Alternatively, we can use a bumpversion.sh or extend git command, something like: git sass-tag M.m.p which will update these files, create tag and also create a commit -- exactly how npm version command works.

@@ -18,14 +18,14 @@ BEGIN
BEGIN
BLOCK "080904b0"
BEGIN
VALUE "CompanyName", "Libsass Organization"
VALUE "CompanyName", "LibSass Organization"
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Sass Open Source Foundation" from the license?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix. Thanks.

MinimumVisualStudioVersion = 10.0.40219.1
Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "libsass", "libsass.vcxproj", "{E4030474-AFC9-4CC6-BEB6-D846F631502B}"
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = ".SolutionItems", ".SolutionItems", "{33318C77-2391-4399-8118-C109155A4A75}"
ProjectSection(SolutionItems) = preProject
..\.editorconfig = ..\.editorconfig
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice, does this actually pick up the setting when you include it here? I was having problems when editing in VS the other day because it wasn't respecting it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to be working. I installed editorconfig for VS extension, opened libsass/win/libsass.sln, set indent_size to 12, opened libsass/src/file.cpp and Ctrl+K,D to format the document, the intended 12 space indentation applied.

Note starting with VS2017, editorconfig and cmake have built-in support [ref].

@@ -1,10 +1,21 @@

Microsoft Visual Studio Solution File, Format Version 12.00
# Visual Studio 2013
VisualStudioVersion = 12.0.30723.0
# Visual Studio 14
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we dropping 2013 as the base version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think 2015 sln file works with 2013 and 2012, but not the lower ones. Besides, from command line perspective, msbuild adapts to the installed toolchain.

Now that things are moving fast with the advent of VS Code for windOS and Unices, the previewed full VS for Mac and previewed VS2017, I honestly see no point in sticking with 2013 when we still can provide better support from XP-10 and all related server editions with latest CRT/CPP toolchains (brining dozens of bugfixes in STL and standard conformance). If there is a bug refraining us to moving forward, like any other piece of software, lets report the issue upstream, track it in our repo and keep the dice rolling forward when we are no backward compatibility trade-offs. Officially VC team hasn't even dropped XP support yet (released: 24.04.2001).

My €0.2 :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty sure you're correct, since it's only the project toolsversion stuff that breaks backwards compat. Maybe AppVeyor should be updated too (separately)

os: Visual Studio 2013

Copy link
Contributor

Choose a reason for hiding this comment

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

Due to issues with Node Sass we need to keep CI at VS2013.

VALUE "InternalName", "libsass"
VALUE "LegalCopyright", "©2014 libsass.org"
VALUE "LegalCopyright", "\251 2017 libsass.org"
Copy link
Collaborator

Choose a reason for hiding this comment

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

libsass.org just redirects to sass-lang.com now

Copy link
Contributor

Choose a reason for hiding this comment

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

It redirects to the LibSass landing page. It's worth keeping for now.

@am11 am11 force-pushed the master branch 2 times, most recently from ed018ed to 2689261 Compare January 6, 2017 10:54
@xzyfer
Copy link
Contributor

xzyfer commented Jan 6, 2017

Thanks for looking into this @am11. I don't think we need to complicate this any further. This is the first time we've bumped the C API ABI, so the process is new to me, and undocumented.

@am11
Copy link
Contributor Author

am11 commented Jan 29, 2017

Rebased the branch against master.

@xzyfer
Copy link
Contributor

xzyfer commented Jan 29, 2017 via email

@xzyfer xzyfer merged commit 1849092 into sass:master Jan 30, 2017
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.

3 participants