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.

Sign up for GitHub

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

Jump to bottom

Add SassC to the CI build setup #905

Closed
wants to merge 0 commits into from
Closed

Add SassC to the CI build setup #905

wants to merge 0 commits into from

Conversation

nschonni
Copy link
Contributor

@nschonni nschonni commented Sep 4, 2016

Continuing from #904 since apparently you can't reopen a PR after rebasing and pushing 😦

  • Extended the test matrix to include 3.4, 3.5, and 4.0 with SassC, but added all of them to the allowed failures in the matrix.
  • moved the ci build stuff to a separate script, so the other one can continue to be used by the devs
  • Used out-of-source cloning/builds so no more .gitignore changes

@nschonni nschonni force-pushed the sassc-travis branch 2 times, most recently from 1ab9159 to b6738e4 Compare September 4, 2016 09:41
@xzyfer
Copy link
Contributor

xzyfer commented Sep 5, 2016

I'm not what this is trying to achieve @nschonni, can you please elaborate on your thinking?

@nschonni
Copy link
Contributor Author

nschonni commented Sep 6, 2016

This addresses #852, but I didn't really see it till afterwards 😊
Think I should move the SassC 3.4 out of the allowed failures, but at least you can drill into the 3.5 and 4.0 results to see what tests might be OK to remove from the todo https://travis-ci.org/sass/sass-spec/builds/157422056

saper
saper reviewed Sep 6, 2016
View reviewed changes
script/ci-build

export DIR=`pwd`

if [ "x$RUBY_SASS" = "xyes" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

This ancient trick is necessary when doing $var = yes to avoid a syntax error, "${VAR}" = "yes" should do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I was just following the existing code convention

@nschonni nschonni force-pushed the sassc-travis branch 2 times, most recently from d756a5c to 5d9095f Compare September 16, 2016 05:51
@nschonni
Copy link
Contributor Author

Closing while I work out the matrix

@nschonni nschonni closed this Sep 17, 2016
@nschonni
Copy link
Contributor Author

My Travis is working again. Reopening and then I'll rebase this down

@nschonni nschonni reopened this Sep 17, 2016
@nschonni nschonni force-pushed the sassc-travis branch 4 times, most recently from 9f73bb9 to 50d8b86 Compare September 17, 2016 17:25
@nschonni
Copy link
Contributor Author

Sorry for the extra push noise. When I squashed the commits, I rebased with upstream to grab the latest tests, and that caused some new failures.

@saper
Copy link
Member

saper commented Sep 17, 2016

@nschonni no worries! go ahead with whatever you need to do!

@nschonni
Copy link
Contributor Author

Should be good to review/merge now

@nschonni
Copy link
Contributor Author

@xzyfer any feedback on this one?

xzyfer
xzyfer reviewed Oct 18, 2016
View reviewed changes
.travis.yml

script:
- bundle exec sass-spec.rb -V "$LANGUAGE_VERSION" --probe-todo
- bundle exec sass-spec.rb -V "$LANGUAGE_VERSION" --probe-todo -c "../sassc/bin/sassc" --impl 'libsass' || if [ "$LANGUAGE_VERSION" != "3.4" ]; then exit 0; fi
Copy link
Contributor

Choose a reason for hiding this comment

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

|| if [ "$LANGUAGE_VERSION" != "3.4" ]

This is tricky. LibSass will be landing multiple 3.5 features very soon.
For the time being LibSass should run with 3.5 but be allowed to fail. Not sure how to achieve this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what this is doing right now with this. It allows the tests to fail if it isn't running 3.4. When 3.5 becomes "stable", then I'd change this to [ "$LANGUAGE_VERSION" = "4.0" ] so it would only tollerate failures there.

xzyfer
xzyfer requested changes Oct 19, 2016
View reviewed changes
.travis.yml

script:
- bundle exec sass-spec.rb -V "$LANGUAGE_VERSION" --probe-todo
- bundle exec sass-spec.rb -V "$LANGUAGE_VERSION" --probe-todo -c "../sassc/bin/sassc" --impl 'libsass' || if [ "$LANGUAGE_VERSION" != "3.4" ]; then exit 0; fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove --probe-todo see #932

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, done. I guess we should look at that flag if it's not doing what it's supposed to

@nschonni nschonni mentioned this pull request Nov 8, 2016
Closed
xzyfer
xzyfer requested changes Nov 8, 2016
View reviewed changes
.travis.yml

before_install:
- rm Gemfile.lock
- git clone https://github.com/sass/libsass.git $TRAVIS_BUILD_DIR/../libsass
Copy link
Contributor

Choose a reason for hiding this comment

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

This should fetch the latest tag, not master. Master is unstable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I guess you're thinking about pinning to 3.3.6 then? Anything a release happens, then this travis.yml gets updated? I guess later it can do what Ruby Sass does and try Stable + git.

Copy link
Contributor

Choose a reason for hiding this comment

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

For now it should be pinned to 3.4.0rc tag since that's what we expect with the 3.4 version annotation.

.travis.yml
before_install:
- rm Gemfile.lock
- git clone https://github.com/sass/libsass.git $TRAVIS_BUILD_DIR/../libsass
- git clone https://github.com/sass/sassc.git $TRAVIS_BUILD_DIR/../sassc
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@nschonni
Copy link
Contributor Author

nschonni commented Nov 8, 2016

Maintainer edits are on if anyone else wants to change it now. I'll see if I can do it tonight, but I'm not sure when I'll be back home.

@xzyfer xzyfer force-pushed the sassc-travis branch 3 times, most recently from 0413161 to 47eb26e Compare November 9, 2016 12:35
@xzyfer xzyfer force-pushed the sassc-travis branch 4 times, most recently from 169c841 to f68b9f4 Compare November 9, 2016 12:46
@xzyfer
Copy link
Contributor

xzyfer commented Nov 9, 2016

@nschonni went in a slightly different direction with this. Also had to rebase off master to get #959.

@xzyfer
Copy link
Contributor

xzyfer commented Nov 9, 2016

Jumping into bed now. Feel free to ship if CI is green when you're online.

nschonni
nschonni commented Nov 9, 2016
View reviewed changes
Copy link
Contributor Author

@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.

Think this approach is good. The original might be a little easier to add dart-sass as another grid though

test_all_impls.sh
@@ -1,17 +0,0 @@
#!/bin/bash
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 this was a local dev tools, that's why I left it before

.travis.yml

before_install:
- rm Gemfile.lock
- if [ $IMPL == "libsass" ]; then
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 this makes more sense

@nschonni
Copy link
Contributor Author

nschonni commented Nov 9, 2016

CI failures look like legitimate failures. You'll need to land this if you want since I can't merge in this repo.

@xzyfer xzyfer force-pushed the sassc-travis branch 2 times, most recently from e7fa3ea to 8ad74da Compare November 10, 2016 08:58
@xzyfer xzyfer closed this Nov 10, 2016
@xzyfer xzyfer mentioned this pull request Nov 10, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xzyfer xzyfer xzyfer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nschonni @xzyfer @saper