sass / libsass

A C/C++ implementation of a Sass compiler
https://sass-lang.com/libsass
Other
4.33k stars 463 forks source link

Use covariant return types in the visitors #382

Closed akhleung closed 9 years ago

akhleung commented 10 years ago

Currently there's a lot of downcasting in the visitor methods ... you can get rid of the casts by using covariant return types.

mgreter commented 9 years ago

@akhleung Can you give more details what your specific idea was?

akhleung commented 9 years ago

Give me a couple of days to collect my thoughts -- it's been a while, and I'll need to revisit some of the code in the visitors (no pun intended).

mgreter commented 9 years ago

I found another optimization for prelexer sequence and alternatives with variadic templates:

// Tries the matchers in sequence and succeeds if they all succeed.
template <prelexer... mxs>
const char* alternatives(const char* src) {
  const char* rslt;
  for (prelexer mx : { mxs... }) {
    if ((rslt = mx(src))) return rslt;
  }
  return 0;
}

// Tries the matchers in sequence and succeeds if they all succeed.
template <prelexer... mxs>
const char* sequence(const char* src) {
  const char* rslt = src;
  for (prelexer mx : { mxs... }) {
    if (!(rslt = mx(rslt))) return 0;
  }
  return rslt;
}
xzyfer commented 9 years ago

I'm all for this stuff :+1: but as you said @mgreter

I would speak against using the full feature set of cpp11, it already intoduced some headaches for the node-sass people since they provide prebuilt binaries. Some people still seem to be on systems which only provide gcc 4.4. ... source https://github.com/sass/libsass/issues/745#issuecomment-67813306

If we head down this track we're quickly going to hit a point of no return for old centos

mgreter commented 9 years ago

@xzyfer IMO this is a pretty neat improvement since it removed about 100 lines of codes and made the sequence and alternatives prelexer accept any number of arguments. If it blows up we always can revert and go back to create those functions by hand when needed.

xzyfer commented 9 years ago

I'm 100% for this. Just concerned how we'll deal with you're CentOS patch for node-sass /cc @am11

am11 commented 9 years ago

Just wondering.. Will we ever be able to move forward with C++11, 14, 17 features, saying goodbye to the old CentOS and the glibc/libstdc++ that goes with it?

am11 commented 9 years ago

Old CentOS is a bad news, kept haunting me for quite some nights. :ghost:

mgreter commented 9 years ago

According to https://gcc.gnu.org/projects/cxx0x.html variadic templates have been included since gcc 4.3, so changes are good that it will compile with pretty old gcc and c++0x (which we do with CentOS patch).

xzyfer commented 9 years ago

Cool, great news. Just playing devil's advocate :)

am11 commented 9 years ago

:+1: :tada:

xzyfer commented 9 years ago

@am11 generally speaking the only reason we're not adopting more C++ 11/14/17 features is for node-sass' CentOS support.

am11 commented 9 years ago

Down the road, if we get attention from someone expert in build-systems, especially gyp, we probably would be able to distill the Linux binary's moving parts, and link all the required machinery statically. Binary size will grow dramatically (from ~2MB to ~5MB each I suppose?) but, IMO, that tradeoff is worthwhile. Speaking of build-systems, hello @QuLogic! :sunglasses:

xzyfer commented 9 years ago

I've done some quick looking into this, and it sounds it should be possible if the binary is compiled on a system with gcc >= 4.5 (http://stackoverflow.com/questions/13636513/linking-libstdc-statically-any-gotchas).

Also looks like problem may be node-sass specific? (not sure, maybe)

$ libsass % ldd lib/libsass.a
        not a dynamic executable
am11 commented 9 years ago

@xzyfer thanks! Is it like diet-glibc that we can statically compile with? Also we would need to static compile libstdc++ as well, which in turn depends on system glibc which is another pain-point. :(

PS: I think we cannot compile glibc and libstdc++ together (I read it somewhere).

xzyfer commented 9 years ago

/cc @Vektah

vektah commented 9 years ago

@xzyfer There are two nice approaches mentioned here: https://sourceware.org/ml/libc-help/2011-04/msg00034.html

Namely:

Resultant executable should run on both new and old versions of glibc.

vektah commented 9 years ago

Turns out options 2 is pretty damn easy with docker.

create a Dockerfile:

FROM centos:6

RUN curl -SL http://people.centos.org/tru/devtools-1.1/devtools-1.1.repo > /etc/yum.repos.d/devtools-1.1.repo
RUN curl -sL https://rpm.nodesource.com/setup | bash -
RUN yum --enablerepo=testing-1.1-devtools-6 install -y devtoolset-1.1-gcc devtoolset-1.1-gcc-c++ nodejs git
RUN git clone https://github.com/sass/node-sass

WORKDIR /node-sass
RUN npm install
RUN git submodule update --init --recursive
RUN PATH="/opt/centos/devtoolset-1.1/root/usr/bin/:$PATH" node scripts/build.js

then run:

docker build . -tag node-sass-bin
docker run node-sass-bin cat /node-sass/vendor/linux-x64-11/binding.node > binding.node

The resulting binding.node is linked against the older glibc and runs on both ubuntu and centos6!

am11 commented 9 years ago

Wohoo! Thanks @Vektah ! Is CentOS 5 also supported? Because that is our least minimum. (-8

vektah commented 9 years ago

Surprisingly, it will work. One binary spanning 8 years of linux:

Modified Dockerfile:

FROM centos:5

RUN yum install -y curl git make
RUN rpm -ivh http://dl.fedoraproject.org/pub/epel/5/x86_64/epel-release-5-4.noarch.rpm
RUN curl -SL https://rpm.nodesource.com/setup | bash -
RUN curl -SL http://people.centos.org/tru/devtools/devtools.repo > /etc/yum.repos.d/devtools.repo
RUN yum --enablerepo=testing-devtools-5 install -y devtoolset-1.0 nodejs
RUN git clone https://github.com/sass/node-sass

WORKDIR /node-sass
RUN npm install
RUN git submodule update --init --recursive
RUN rm /usr/bin/python && ln -s /usr/bin/python26 /usr/bin/python # This centos install is probably toast at this point... It will break yum and other things from here out.
RUN PATH="/opt/centos/devtoolset-1.0/root/usr/bin:$PATH" node scripts/build.js
[root@8839b0c4d367 node-sass]# cat /etc/redhat-release 
CentOS release 5.11 (Final)
[root@8839b0c4d367 node-sass]# ./bin/node-sass --output-style compressed foo.scss 
body{font:100% Helvetica,sans-serif;color:#333}

and same binding.node:

% cat /etc/lsb-release 
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=14.10
DISTRIB_CODENAME=utopic
DISTRIB_DESCRIPTION="Ubuntu 14.10"
% ./bin/node-sass --output-style compressed foo.scss 
body{font:100% Helvetica,sans-serif;color:#333}
am11 commented 9 years ago

This is brilliant! Thanks. :+1:

I tried to compile GCC 4.7 on CentOS 5.5. and 5.11, but it failed. Which version of GCC it installed after curl -SL http://people.centos.org/tru/devtools/devtools.repo > /etc/yum.repos.d/devtools.repo?

QuLogic commented 9 years ago

RUN rm /usr/bin/python && ln -s /usr/bin/python26 /usr/bin/python # This centos install is probably toast at this point... It will break yum and other things from here out.

Wouldn't it make more sense to run something like scl enable python26 bash? Well, I mean that's how it should work in newer CentOS; I'm not really sure about 5...

vektah commented 9 years ago

@am11 The curl does not install gcc it just adds another repo

yum --enablerepo=testing-devtools-5 install -y devtoolset-1.0

Installs g++ in /opt:

[root@5f5088f9baa3 node-sass]# /opt/centos/devtoolset-1.0/root/usr/bin/g++ --version
g++ (GCC) 4.7.0 20120507 (Red Hat 4.7.0-5)
Copyright (C) 2012 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Its not the newest, but it does compile master. It also does not change the system version of gcc, so both can be installed side by side in a normal centos distro.

@QuLogic I assumed there was a better way of doing that, but didn't go looking. Sadly it does not seem to work:

Step 10 : RUN scl enable python26 bash
 ---> Running in dd8b486efc27
Unable to open /etc/scl/prefixes/python26!

The docker images are pretty minimal, as you can see I needed to install curl. The comment probably applies either way as things like yum have #!/usr/bin/python.

am11 commented 9 years ago

@Vektah, yeah I copied the line before. :-1:

Incidentally, can we have x86 tooling there as well or should we have a separate Docker image for x86 arch?

mgreter commented 9 years ago

@akhleung I'm going to close this! Please feel free to open a new issue if you have more details!