Skip to content

Use covariant return types in the visitors #382

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

Closed
akhleung opened this issue Jun 5, 2014 · 25 comments
Closed

Use covariant return types in the visitors #382

akhleung opened this issue Jun 5, 2014 · 25 comments

Comments

@akhleung
Copy link

akhleung commented Jun 5, 2014

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

@mgreter
Copy link
Contributor

mgreter commented Mar 9, 2015

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

@akhleung
Copy link
Author

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
Copy link
Contributor

mgreter commented Mar 15, 2015

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
Copy link
Contributor

xzyfer commented Mar 15, 2015

I'm all for this stuff 👍 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 #745 (comment)

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

@mgreter
Copy link
Contributor

mgreter commented Mar 17, 2015

@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
Copy link
Contributor

xzyfer commented Mar 17, 2015

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

@am11
Copy link
Contributor

am11 commented Mar 17, 2015

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
Copy link
Contributor

am11 commented Mar 17, 2015

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

@mgreter
Copy link
Contributor

mgreter commented Mar 17, 2015

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
Copy link
Contributor

xzyfer commented Mar 17, 2015

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

@am11
Copy link
Contributor

am11 commented Mar 17, 2015

👍 🎉

@xzyfer
Copy link
Contributor

xzyfer commented Mar 17, 2015

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

@am11
Copy link
Contributor

am11 commented Mar 17, 2015

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! 😎

@xzyfer
Copy link
Contributor

xzyfer commented Mar 18, 2015

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
Copy link
Contributor

am11 commented Mar 18, 2015

@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
Copy link
Contributor

xzyfer commented Mar 18, 2015

/cc @vektah

@vektah
Copy link

vektah commented Mar 18, 2015

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

Namely:

  • build a gcc 4.9 cross compiler that targets the version of glibc on centos.
  • build a recent gcc in a centos vm/docker environment and use that to link against the old version of gcc.

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

@vektah
Copy link

vektah commented Mar 19, 2015

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
Copy link
Contributor

am11 commented Mar 19, 2015

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

@vektah
Copy link

vektah commented Mar 19, 2015

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
Copy link
Contributor

am11 commented Mar 19, 2015

This is brilliant! Thanks. 👍

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
Copy link
Contributor

QuLogic commented Mar 19, 2015

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
Copy link

vektah commented Mar 19, 2015

@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
Copy link
Contributor

am11 commented Mar 20, 2015

@vektah, yeah I copied the line before. 👎

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

@mgreter
Copy link
Contributor

mgreter commented Mar 30, 2015

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

@mgreter mgreter closed this as completed Mar 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants