Update SASS ( 3.3.5), Bourbon (4.0.2), Neat (1.6.0)

[andy-armstrong]

This work aims to update our Sass library dependency to 3.3.5. This significant Sass version requires an update of our other Sass dependencies as well (Bourbon and Neat).

---

Note: this requires the following:

• SASS 3.3+
• Bourbon 4.0.2
• Neat 1.6.0

so do bundle install

[andy-armstrong]

@talbs I started to look into source maps but real life intervened. I'm not sure yet how to get Jenkins to pick up the new version of SASS, so all the builds are failing. I also don't see the source maps being generated on my devstack, so I'm clearly doing something wrong. Hopefully I'll get back to this soon, but if you want to run with it...

[talbs]

Thanks for getting this started, @andy-armstrong! Its much appreciated.

I'll take a look locally next week when I'm back in the office. In the meantime, I've started two PRs into this branch that take advantage of our update to Sass 3.3+ and update our Sass Utilities:

• Update to Bourbon - https://github.com/edx/edx-platform/pull/3469
• Update to Neat - https://github.com/edx/edx-platform/pull/3470

As far as next steps, @singingwolfboy may be a good resource to hit up on how to test the updates locally and anything we may be missing in general when updating gem versions (he helped with the last Sass gem update).

Let me know if you have any questions or concerns in the meantime.

[talbs]

Also, pinging @feanil on this work as he's interested in helping make the pipeline and our local front end asset compilation better.

[jzoldak]

There might be a bit of jenkins worker config to do to get these 3 PRs to build. And after merging make sure that other branches that do not have the commit can build also (or require people to rebase or cherry-pick).

[benpatterson]

I've updated Jenkins worker config and we should be able to support this now. I'll need to make an additional change to Gemfile.lock (will push to this PR in a bit) and I can review the job results after that is picked up by Jenkins. Discussed that change briefly with @andy-armstrong.

[andy-armstrong]

Thanks @benpatterson. There was no way I was going to be doing that myself. :-)

[benpatterson]

@andy-armstrong the tests pass with the latest commit + a rebase of master (and of course the Jenkins infrastructure work)

[andy-armstrong]

Thanks, @benpatterson. Before we can merge this we need to work out the steps that developers should follow when rebasing to this commit. Is that something you can help with? If not then I'll look into when I get a chance, but it won't be for a day or two.

[benpatterson]

@andy-armstrong I also couldn't get to it for a couple of days. If it's any help, running bundle install will update everything properly now, so hopefully it won't be too great an adjustment. Verifying that for sure though, is not something I could help with just yet.

[andy-armstrong]

If all it takes is bundle install then that's great. I'll try it when I get a moment and I'll get back to you.

[benpatterson]

Removing testeng label from the issue; everything should be in place now to support continued development of this PR.

[feanil]

@andy-armstrong have you tried building this branch on a single instance sandbox to make sure that there are no issues in a production-like environment? Looks like the changes to the asset command are pretty minor but it would be good to make sure on a single instance sandbox. Other than that the changes look good to me.

[andy-armstrong]

@feanil I've built my sandbox from this branch and my smoke testing makes it seem okay.

http://studio.andy-armstrong.m.sandbox.edx.org/course

Do you have any testing in particular you wanted me to do? @talbs, how about you?

I've also rebased the branch and am testing it locally. If I don't see any issues then I'll merge this later today.

[benpatterson]

@andy-armstrong from my end, we were seeing lots of deprecation warnings. (Just warnings, but a bunch of them). Not sure if those need to be addressed or if they result in different functionality than you're expecting with the upgrade, for example.

[talbs]

@benpatterson, do you have any output of those warnings? They may be Sass syntax warnings - if so, I can take a look.

[feanil]

Looks like @talbs is updating the bourbon and neat versions as well. Operationally, I think the requirements are that you are able to run this code on a single instance sandbox and all assets compile and are server. As long as the output is still going through the django pipeline, I don't have any issues.

[benpatterson]

@talbs Yes I see those warnings on one of the current test builds. Here's the console output for one of them. You'll see them a couple pages down. https://jenkins.testeng.edx.org/job/edx-all-tests-auto-pr/11263/SHARD=2,TEST_SUITE=lms-acceptance/console

An example:

16:47:51 DEPRECATION WARNING on line 32 of /home/jenkins/workspace/edx-all-tests-auto-pr/SHARD/2/TEST_SUITE/lms-acceptance/common/static/sass/bourbon/css3/_keyframes.scss:
16:47:51 Assigning to global variable "$prefix-for-spec" by default is deprecated.
16:47:51 In future versions of Sass, this will create a new local variable.
16:47:51 If you want to assign to the global variable, use "$prefix-for-spec: true !global" instead.
16:47:51 Note that this will be incompatible with Sass 3.2.

[talbs]

@benpatterson, thanks!

I've talked with @andy-armstrong, and I think now that my bourbon/neat updates are merged in, these warnings should be eliminated. Andy's rebuilding his sandbox with these latest commits and we'll check for warnings once that's done.

[benpatterson]

Right on @andy-armstrong @talbs. Thanks for the heads up. Do your thang.

[andy-armstrong]

Hmm, the sandbox didn't build correctly. It just says "Internal Server Error". I don't have time to investigate right now so if someone else can jump in that would be great. Otherwise I'll try to get back to it tomorrow.

http://studio.andy-armstrong.m.sandbox.edx.org/

[talbs]

Update: Sandbox seems to be working - http://studio.andy-armstrong.m.sandbox.edx.org/ and http://andy-armstrong.m.sandbox.edx.org/ load fine.

[talbs]

Update: I realized now that the gems are updated, I need to do the following on the Bourbon/Neat side:

• update all of the common/static/sass/bourbon and common/static/sass/neat files from the gems
• squash all of the Sass warnings (things seem to operate fine on a rendering level, but I'd rather not go forward with all of these messages)

I'll update this PR once those two items have been completed.

[talbs]

Update: I've taken some time to:

• update the bourbon/neat-generated static Sass files.
• resolve any deprecation warnings that either the newer versions of Sass or Bourbon were specifically giving us with some of our older platform syntax (warning should not show up locally while compiling now)
• update us to Bourbon 4.0.2 (the latest and greatest)

[andy-armstrong]

@talbs That's a whole lotta Sass changes. They seem fine to me, but probably you should get a UX reviewer. 👍 from me.

[talbs]

This should be ready to review and merge now. Here's a sandbox with the branch running - http://talbs.m.sandbox.edx.org/ and http://studio.talbs.m.sandbox.edx.org/

@andy-armstrong, @benpatterson, and @frrrances, mind taking a look and making sure things look solid?

[talbs]

Thanks, @andy-armstrong. I had to scope the names of those xmodule variables to not override our global and well-planned variables with the same names. its not ideal to have these now specifically still named vars still hanging out, but I'd imagine the switch to xblocks or a general FED cleaning would take care of these.

[benpatterson]

Still getting familiar with paver, but will this still work with the comma?

[singingwolfboy]

That's not a Paver thing, it's a Python thing -- and yes, trailing commas are ignored.

[benpatterson]

Nice. Thx.

[benpatterson]

@talbs It looks good to me from my end 👍 though I also would defer to a UX expert to verify the style changes.

[frrrances]

lgtm. 👍

[talbs]

Thanks for the double checks, all.

Being the fella who's made the more active changes as of late and after proofing local/sandbox versions of the work, I'm going to 🚚 this fella to Mergetown.

[talbs]

Folks can now rock this - https://developer.chrome.com/devtools/docs/css-preprocessors