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

Add Windows CI #189

Merged
merged 1 commit into from
Sep 14, 2016
Merged

Add Windows CI #189

merged 1 commit into from
Sep 14, 2016

Conversation

am11
Copy link
Contributor

@am11 am11 commented Sep 7, 2016

  • With AppVeyor badge in README.
  • Ability to store artifacts.
  • Configured tests but disabled them (with a code comment how to enable
    them) because:
    • Some tests were failing.
    • Unix script doesn't run the tests as well.
    • Test takes too much time to run.
  • Fixed vcxproj file making LIBSASS_DIR setting conditional. The new
    precedence order is:
    • LIBSASS_DIR property in vcxproj file can be overridden by
      environment variable of same name (see appveyor.yml for example).
    • /p:LIBSASS_DIR=<relative-path> command-line argument to MSBuild
      can override environment variable hence have the highest precedence.
  • Note that the value of LIBSASS_DIR must be set to a relative path
    based at the location of sassc.sln file; absolute paths are not
    supported (yet).
  • Removed some statements from .travis.yml and defined global
    variable instead.
  • Updated windows build docs.

@am11
Copy link
Contributor Author

am11 commented Sep 7, 2016

AppVeyor CI enabled for PRs. (note: testless builds just like Travis CI)

Also, the dumpbin command is to counter Unix ldd command in .travis.yml. ✈️

@@ -3,7 +3,7 @@
<PropertyGroup>
<SASSC_VERSION>[NA]</SASSC_VERSION>
<LIBSASS_VERSION>[NA]</LIBSASS_VERSION>
<LIBSASS_DIR>..\..</LIBSASS_DIR>
<LIBSASS_DIR Condition="'$(LIBSASS_DIR)' == ''">..\..</LIBSASS_DIR>
Copy link
Contributor

@mgreter mgreter Sep 7, 2016

Choose a reason for hiding this comment

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

Is this condition really needed? My tests indicated that it can still be overloaded by command line arg /P:LIBSASS_DIR=.... Or is this related to environment variables? Edit: I guess it is so from reading the details in the main PR info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, the default order is:

  • property defined by command line arg has highest precedence.
  • property defined by environment variable has lowest precedence.
  • property defined in vcxproj can override environment variable.

This change makes that defined in vcxproj the lowest precedence; rendering environment variable being able to override it, while keeping the CLI arg highest precedence.

This is used in AppVeyor.yml in its current form. Otherwise, we will need to change build to build_script in AppVeyor and manually invoke the msbuild /p:LIBSASS_DIR win/sassc.sln command.

@xzyfer
Copy link
Contributor

xzyfer commented Sep 8, 2016

Ability to store artifacts

Is it possible to automatically add the artefacts to GH releases like we do for node sass?

@nschonni
Copy link
Contributor

nschonni commented Sep 8, 2016

👍 @am11 take a look at nschonni@71293f7 (but ignore the bootstrap.cmd stuff since it still isn't working). I think using the existing SASS_LIBSASS_PATH varaible might be better, since it is already used by the other builds.

@am11
Copy link
Contributor Author

am11 commented Sep 8, 2016

@xzyfer,

Is it possible to automatically add the artefacts to GH releases like we do for node sass?

Sure I will investigate it for both Travis and AppVeyor. Opened #190 to track.

@nschonni,

I think using the existing SASS_LIBSASS_PATH

I actually used that name, then reverted it upon realizing that we would need to update libsass/win/libsass.[vcxproj/filters] as well. And then the downstream repos which have taken dependency on our build infra (https://github.com/darrenkopp/libsass-net comes to mind). Nonetheless, it is not an impossible task; your script approach makes the build configuration more streamlined . Should I incorporate the bootstrap.cmd as part of this PR, after merging this or else how would you like to pursue it?

@xzyfer
Copy link
Contributor

xzyfer commented Sep 8, 2016

@am11 we have a working implementation in node-sass you can work from. You're amazing :)

@am11 am11 force-pushed the master branch 11 times, most recently from 4d6ed6f to c14312d Compare September 10, 2016 00:15
@am11
Copy link
Contributor Author

am11 commented Sep 10, 2016

@xzyfer, auto deployment has been configured the same way as node-sass (fixed #190). :)

e.g. https://github.com/am11/sassc/releases/tag/3.3.11 was deployed by https://ci.appveyor.com/project/am11/sassc/build/1.0.72. When pushed to Release branch followed by the tag push triggers the tag build which uploads the artifacts as release assets. Note that it will make a draft release, which can then be published manually (giving human time to write release notes and test the build artifacts before pushing the publish button). If draft is not required, we can just remove draft: true line.

I don't have access to create a branch or push tags on sassc repo; probably the admin would need to generate a new auth key and update it in the YAML file.

@xzyfer
Copy link
Contributor

xzyfer commented Sep 12, 2016

Incredible work as always @am11. I've added you @sass/sassc. Feel free to merge when you're confident.

@@ -0,0 +1,96 @@
-
branches:
only:
Copy link
Contributor

Choose a reason for hiding this comment

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

If this can be configured to only build on tags that'd be great. These branches were a work around due to a weird timing bug with the node-sass appveyor setup. Turned out we were still firing a webhook to your personal node-sass appveyor. That was causing issues with our webhooks timing. Removing that made the release branch unnecessary.

@xzyfer
Copy link
Contributor

xzyfer commented Sep 12, 2016

Creating drafts is good. We should do the same in node-sass.

@am11 am11 force-pushed the master branch 5 times, most recently from 1007640 to b95485e Compare September 12, 2016 18:29
@xzyfer
Copy link
Contributor

xzyfer commented Sep 13, 2016

Incredible work @am11. Feel free to merge when you're ready.

@am11
Copy link
Contributor Author

am11 commented Sep 13, 2016

Thanks @xzyfer! 🍰

I have a question for AppVeyor team: appveyor/ci#1039. The only (tiny) downside of this simplified approach -- where we have unified configuration for tag-push and commit-push -- is that it does not skips the Debug build for tag push although we have a constraint suggesting "deploy only Release build when tags are pushed". Perhaps there exists proper skip semantics to achieve the desired behaviour. Once we get a response, I will fix up and merge.

+1 for the draft release by auto-deploy in node-sass. :)

@am11 am11 force-pushed the master branch 11 times, most recently from 5290fc8 to e9ce2d2 Compare September 14, 2016 01:19
* With AppVeyor badge in README.
* Ability to store artifacts.
* Configured tests but disabled them (with a code comment how to enable
  them) because:
  * Some tests were failing.
  * Unix script doesn't run the tests as well.
  * Test takes too much time to run.
* Fixed vcxproj file making `LIBSASS_DIR` setting conditional. The new
  precedence order is:
  * `LIBSASS_DIR` property in `vcxproj` file can be overridden by
    environment variable of same name (see appveyor.yml for example).
  * `/p:LIBSASS_DIR=<relative-path>` command-line argument to MSBuild
    can override environment variable hence have the highest precedence.
* Note that the value of `LIBSASS_DIR` *must* be set to a relative path
  based at the location of `sassc.sln` file; absolute paths are not
  supported (yet).
* Removed some statements from `.travis.yml` and defined global
  variable instead.
* Updated windows build docs.
@am11
Copy link
Contributor Author

am11 commented Sep 14, 2016

The deployment will take place upon pushing the tag made off the master branch only.

appveyor/ci#1039 changes will be incorporated later based on the answer we will receive.

@am11 am11 merged commit 3b230a8 into sass:master Sep 14, 2016
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.

4 participants