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.

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

Replace pake with make? #3527

Merged
merged 8 commits into from Apr 26, 2015
Merged

Replace pake with make? #3527

merged 8 commits into from Apr 26, 2015

Conversation

elemoine
Copy link
Member

@elemoine elemoine commented Apr 9, 2015

My makefile branch replaces pake by GNU make. The migration is not complete (but not so far either), but I'd like to know what people think about this before going further.

Replacing the build.py script by a Makefile would allow for a more compact file, and would make us rely on a mainstream tool instead of pake.

The question is: do people want to use a tool like make? Or do people think we'd do just fine with just node, npm and npm tasks? These questions are related to my comment in #3504.

Personally, I'd like to use make, because it's very nice to do make check and have make figure out the minimum set of things that needs to be rebuilt.

@marcjansen
Copy link
Member

I am personally not at all opposed to make. I don't know so much node / npm so that I could decide if they can be used to accomplish sth. similar than what we have with pake or what we would get with make.

@pagameba
Copy link
Member

The biggest requirement in my mind is that the build system work reliably for users on all platforms that are supported.

@elemoine
Copy link
Member Author

Current Makefile is very close to feature parity with build.py. It is also much more concise.

@elemoine
Copy link
Member Author

The biggest requirement in my mind is that the build system work reliably for users on all platforms that are supported.

I haven't tested it myself, but some people reported to me that http://gnuwin32.sourceforge.net/packages/make.htm worked great for them.

And, as indicated in #3532, Windows users should be able to get by with npm install, npm test, and npm start. So I don't think this PR is incompatible with #3532.

@ThomasG77
Copy link
Contributor

I would suggest to use Shelljs. The official description is Portable Unix shell commands for Node.
It supports a syntax similar to Makefile. It's well supported (around 2300 stars on Github)

Best of both worlds:

  • Unix like tools simplicity
  • no limitations due to Make support difference between OS, thanks to Node

@tschaub
Copy link
Member

tschaub commented Apr 13, 2015

+1 to replacing pake with make (with caveats)

As described in #3532, I'd like it if npm install, npm test, and npm start could be cross-platform with minimal system dependencies. Specifically, I don't think these should run anything but Node based tasks.

The generate-requires.js task needs a bit of discussion if we are going to meet this objective. As it is, it is not practical to run the task without make (and find). One alternative would be to make it accept a directory path instead of a list of file paths. I don't think we'd suffer that much without make's mtime comparison here. But I also don't think this task is necessary, so it may not be the best example. Anyway, I'm in favor of using make instead of pake. But I think this will involve a bit more reworking of tasks.

@elemoine
Copy link
Member Author

As described in #3532, I'd like it if npm install, npm test, and npm start could be cross-platform with minimal system dependencies. Specifically, I don't think these should run anything but Node based tasks.

Cool.

The generate-requires.js task needs a bit of discussion if we are going to meet this objective. As it is, it is not practical to run the task without make (and find). One alternative would be to make it accept a directory path instead of a list of file paths. I don't think we'd suffer that much without make's mtime comparison here. But I also don't think this task is necessary, so it may not be the best example.

Yep. I'll wait a bit to see what you come up with regarding the execution of the unit and rendering tests, and whether the test_requires.js and test_rendering_requrires.js files are required.

I'd be interested to know about the other tasks that you think need reworking. Except generate-requires.js, everything looks good to me.

Note that I also added bin/check-requires.py and bin/check-whitespace.py python scripts. These are extractions from build.py. They are now run from the Makefile. Later, we can possibly rewrite them in node/js, but that's certainly not something I want to do now.

@elemoine
Copy link
Member Author

The Makefile is now at feature-parity with build.py. All the build.py targets have been ported, and the Python functions that were in build.py are now separate Python scripts (placed in the bin directory).

I am going to watch @tschaub's work (#3558 for example), and will port his changes to the Makefile.

We will also need to modify the Makefile when we make npm test work without generating "requires" files. This is related to @tschaub's comment above.

@tschaub, it sounds like we need to coordinate.

@elemoine
Copy link
Member Author

Changes to build.py from #3558 now ported to the Makefile.

Do people still agree with moving from pake to make as our "advanced" build tool?

@elemoine
Copy link
Member Author

Anyone opposed to merging this? If/when this is merged I'll go ahead and change openlayers.github.io to use make instead of build.py. And when this is done I'll come with a new ol3 PR for removing build.py entirely.

@tonio
Copy link
Member

tonio commented Apr 20, 2015

not opposed at all.

@fredj
Copy link
Member

fredj commented Apr 20, 2015

+1

@elemoine
Copy link
Member Author

There were remaining issues on OS X. They are now fixed.

@elemoine
Copy link
Member Author

I updated CONTRIBUTING.md and other files to replace build.py by make. I also decided to remove build.cmd, build.py and pake.py as part of this pull request.

When this is merged I'll merge openlayers/openlayers.github.io#51 and immediately re-build the website to verify that everything works as expected.

As commented by @tschaub we ultimately want npm install, npm test and npm serve to be cross-platform with minimal system dependencies This, in particular, means that having the serve.js task, instead of the Makefile, generate the test_requires.js and test_rendering_requires.js files.

Using the Makefile is similar to using build.py. For example, to start the development server you'll just use make serve instead of build.py serve. And to run the code checkers you'll use make check instead of build.py check.

You can run make help (or just make) to get a list of the most common targets. This is the current output of make help:

$ make

The most common targets are:

- install                 Install node dependencies
- serve                   Start dev server for running examples and tests
- test                    Run unit tests in the console
- check                   Perform a number of checks on the code
- clean                   Remove generated files
- help                    Display this help message

Other less frequently used targets are:

- build                   Build ol.js, ol-debug.js, ol.js.map and ol.css
- lint                    Check the code with the linter
- ci                      Run the full continuous integration process
- apidoc                  Build the API documentation using JSDoc
- cleanall                Remove all the build artefacts
- check-deps              Check if the required dependencies are installed

elemoine pushed a commit that referenced this pull request Apr 26, 2015
@elemoine elemoine merged commit c025f63 into openlayers:master Apr 26, 2015
@elemoine elemoine deleted the makefile branch April 26, 2015 19:12
@tschaub tschaub mentioned this pull request May 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants