Move source files into src subfolder

[mgreter]

Move sources into sub folder and other code reorganizations (only last commits are relevant). Waiting for other PRs to be merged, so this can be rebased! For now I also had to use my local sassc repo.

[drewwells]

This is going to cause a lot of problems for me. Is there a specific entrypoint to include c/h files? Writing a cpp file like

mega.cpp

import ast.cpp
...
import sass_value.cpp

Just creates a broken compile time situation. Can as part of this some sort of import source/header that includes all files in the correct order?

[mgreter]

The only headers you should include are the ones that will be in include subfolder (only C-API headers). I really hope you don't import any of the hpp headers, since they are not meant for consumers to be included. Also don't see how this could cause much problems, since you just need to pass the new include path to the compiler (mostly -I include).

I guess it will take me 5 minutes to update perl-libsass to use the new file structure!

sass_context.h should include all available headers!
https://github.com/sass/libsass/blob/master/sass.h#L45
https://github.com/sass/libsass/blob/master/sass_context.h#L6

I'm also thinking about moving the headers into a libsass subdirectory, since this is how pretty much all other system libraries handle/install their headers (#import <libsass/sass_context.h>).

[drewwells]

I build libsass from source since it makes the Go binary more portable.

go get github.com/wellington/wellington/wt compiles libsass C/C++, links
C-API to Go, then builds the final Go binary. I've tried amalgamating
libsass, but it ends up with namespace conflicts.

On Thu, Jul 16, 2015 at 1:20 PM Marcel Greter notifications@github.com
wrote:

The only headers you should include are the ones that will be in include
subfolder (only C-API headers). I really hope you don't import any of the
hpp headers, since they are not meant for consumers to be included. Also
don't see how this could cause much problems, since you just need to pass
the new include path to the compiler (mostly -I include).

I guess it will take me 5 minutes to update perl-libsass to use the new
file structure!

—
Reply to this email directly or view it on GitHub
#1352 (comment).

[mgreter]

perl-libsass builds it from source too. Can't you just pass another include directory to the invoked compiler? Seems to be one of the most common cases IMO! Or I don't understand your concerns!!

[xzyfer]

@mgreter I'd prefer to avoid this for now. WE should come back to this after 3.4 when we have parity. For I the over head this is going to introduce when going through git history to find issues/regressions has more cost than benefits here.

[drewwells]

I'll check out your perl-sass setup and see if it helps. Thanks!

[mgreter]

@xzyfer this should all be unproblematic!

a) there is one commit that just moves the files. So we don't lose history or anything!
b) it is how pretty much all other projects are organized and it helps keeping root clean!
c) Compilation only needs two changes: new path for sources and additional include directories!

[xzyfer]

🚢 when 🚥 is ready

[mgreter]

Here the commit needed to adjust perl-libsass: sass/perl-libsass@cdb2c44

[am11]

Looks pretty reasonable to me. 👍

[mgreter]

Merged changes needed for sassc spec runner to pass CI ... let's wait and see 🚥
BTW. changes in sassc are backward compatible, so it also works with the old source tree 😉

[saper]

Hmm, not sure I love it but I think there are few things to consider:

1. Can we fix the Makefile situation? It is highly annoying to have a version-controlled Makefile overwritten by configure

2. lib directory will be confusingly not used in the libtool case, correct? Can we maybe use src/.libs instead?

The reason why it may be useful - I am compiling node-sass bindings with library paths set to $HOME/sw/libsass/.libs/libsass.so. This way I can test changes in libsass without touching the node-sass bindings. Would love to have this location for both autoconf and non-autoconf cases.

3. I think if this reorganization would serve any real purpose, I'd like to have out-of-tree building possible. MSVC is always doing this, we could have this with GNU make.

4. Shell scripts for CI should be simplified to the bare minimum. A good directory organization should allow this (no cd or ${BASH_SOURCE[0]} there anymore).

Some middle-term thoughts:

5. I think ast.hpp is too big. What about introducing one source file per type (number.cpp, string.cpp, ....) to combine all functions related to one node type into one file (and possibly one class). This could have some additional advantages like getting rid out classes like Output and probably we could have much less dynamic_casts (if any).

[drewwells]

Point 3 sounds related to #1223. It would be nice if this was generated by the build rather than modifying it and creating git submodules changes. Or if there was a standalone make rule to update it without performing a build.

[mgreter]

@saper

1. I don't see a way around it without getting rid of either one. Since mingw does not support autotools easily (and it is also pretty slow to generate the makefiles, if you ofter start with an empty tree). So I don't want to get rid of it. So I don't see a solution for this annoyance.

2. which lib directory are you talking about. The lib will be built in build/lib and installed to $prefix/lib. Not sure what you mean by "not used"? Simply define the correct prefix and do make install, then tell node-sass to use the lib from there!?

3. Agree, but one step at a time. I've read this is not that easy with autotools!

4. Also agree, but cannot fix everything at once 😉

[saper]

re 1) What about having hand-generated Makefile in one directory and autotools-generated Makefile in another? Maybe GNUmakefile can be used somehow (generated....)?

The reason I raise this I want to avoid another move because this will be easier for some other reason (simplifying the script for example).

Here's a simpler https://github.com/saper/sassc/blob/fixautoconf/ci-build file that actually can be made even more simple. And we should almost never need to change the directory in the script if the Makefiles are right...

[mgreter]

@saper I don't really see why 1) is such a big problem. The only annoyance I have with it is that I have to be carefull when changing the makefile without commiting and later running autoreconf (which would overwrite any previous unstaged changes). I also accidentaly commit it once or twice, but nothing a rebase/ammed could fix. Other than that I don't see any issues!?

[mgreter]

Pretty sure CI is going 💚 in a few minutes and I'll 🚢 this. It has been tested on mingw (makefiles via CI) under cmd, mingw under msys (makefiles and autotools), gcc and clang on linux (makefiles and autotools via CI), clang on mac (makefiles and autotools via CI) and MSVC under windows via CI. Also tested building sassc directly under windows in all variations (plus CI tests on linux). Each for shared and static!

[saper]

It is a problem, because git status always shows modified Makefile and sass_version.h - one needs to hand-pick files to commit and be careful not to include those; this contributes to getting dirty version indicator even it is not.

[mgreter]

I guess not much we can do about that for now, I only consider it an annoyance. Maybe you can tell autotools to build the Makefile somewhere else for you? Or use the plain makefiles!?

BTW: https://github.com/sass/libsass looks much better, doesn't it?

[saper]

#1358 moves Makefile.am to GNUmakefile.am (plus fixes some issues with .gitignore)

[drewwells]

It's also an order of operation problem. Sass version is modified in the
source directory not the build directory as the issue above mentions. So
version doesn't work when targeting a new build directory.
On Sun, Jul 19, 2015 at 6:40 PM Marcin Cieślak notifications@github.com
wrote:

#1358 #1358 moves Makefile.am to
GNUmakefile.am (plus fixes some issues with .gitignore)

—
Reply to this email directly or view it on GitHub
#1352 (comment).