pkg/jerryscript: build into '$(BINDIR)' and cleanups by cladmi · Pull Request #9824 · RIOT-OS/RIOT

cladmi · 2018-08-22T15:49:04Z

Contribution description

Build into the BINDIR directory instead of the source repository.
This makes 'clean' work as expected without other intervention.

Also it goes in the direction of having the package source repository
board independent.

I also included cleanup for jerryscript makefiles self described in the commits messages.

Testing procedure

Clean actually now cleans.

rm -rf bin; make; make clean
# Then check
ls bin; git -C bin/pkg/native/jerryscript/ status --porcelain
pkg
?? .git-downloaded

In master we had

ls bin; git -C bin/pkg/native/jerryscript/ status --porcelain
pkg
?? .git-downloaded
?? riot/

Where riot contained all the build files.

Issues/PRs references

Part of #9820

cladmi · 2018-08-22T22:05:41Z

@cgundogan any idea why changing this -H to an absolute directory is required ?

cladmi · 2018-08-23T11:56:18Z

It does not fail on my computer and with docker with a relative path so will retry without the absolute one.

cladmi · 2018-08-23T17:38:52Z

Non parallel build works

BOARDS="slstk3402a" make -C examples/javascript/ TOOLCHAIN=llvm buildtest 'BUILDTEST_MAKE_REDIRECT=>/dev/null'  NPROC=1

Parallel does not

BOARDS="slstk3402a" make -C examples/javascript/ TOOLCHAIN=llvm buildtest 'BUILDTEST_MAKE_REDIRECT=>/dev/null'  NPROC=

Now it is reproducible I can investigate.

cladmi · 2018-08-24T15:20:51Z

Found that it is because I replaced the default target by all to correctly use what was in Makefile.base. But that is actually causing problems as it overwrites jerryscript.a.

I think it should not even include Makefile.base as it is not used.

smlng · 2018-09-25T12:17:39Z

pkg/jerryscript/Makefile.jerryscript

@@ -1,15 +1,14 @@
-BUILD_DIR  ?= $(CURDIR)/riot
+BUILD_DIR  ?= $(BINDIR)/jerryscript


why not use BUILD_DIR ?= $(PKG_BUILDDIR) instead?

First it is not defined there, and also it is the directory where the package is cloned.

As described in the commit message and the title of this PR. It is the goal of this one to move toward out of source build, and all normal modules in RIOT build in $(BINDIR)/module_name.

With out of source build, the package could be cloned only once for all platforms (20M * 120 boards == a lot).

ok, then we definitely need to work on the naming of RIOT directories. Because if PKG_BUILDDIR is the directory where the package is cloned and not build, its very confusing - the name is why I suggested to use that for BUILD_DIR.

Some of the packages are still build into there when they use their own build system.

aabadie · 2018-12-19T11:13:48Z

@cladmi, what is the status of this PR ? It would at least require a rebase

Do not include Makefile.base as 'DIRS' are not used here. Also if 'all' target was to be used, there would be two rules creating `$(BINDIR)/jerryscript.a`. By default `Makefile.jerryscript` executes `libjerry` as it is the first target.

Remove unknown rules from .PHONY. Make 'all' be the default target for jerryscript and execute `all` target. This way no definition order issue could happen in the future.

Build into the BINDIR directory instead of the source repository. This makes 'clean' work as expected without other intervention. Also it goes in the direction of having the package source repository board independent.

cladmi · 2018-12-19T13:36:19Z

I squashed to remove the commits that were more on the finding what to do path.

There are now the 5 first commits (check the order locally and not from github as it uses author date) that are for cleaning up and style changes.
And the 6th commit that changes the build directory to BINDIR/jerryscript.

I could split if wanted.

The test command using llvm in the comment does not work anymore as llvm is blacklisted, so just remove the toolchain for now in this command.

aabadie

Changes are OK and tested locally on Arduino-Zero with success.

All commit messages contains the right explanation for the changes, this helps understanding the changes.

ACK

cladmi · 2018-12-19T15:28:41Z

Thank you for the review.

cladmi added Area: build system Area: Build system Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 22, 2018

cladmi mentioned this pull request Aug 22, 2018

pkg/jerryscript: many cleanups (split PRs) #9820

Closed

4 tasks

cladmi added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 23, 2018

cladmi added this to the Release 2018.10 milestone Aug 23, 2018

cladmi added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 24, 2018

cladmi requested a review from emmanuelsearch September 11, 2018 13:59

cladmi added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 11, 2018

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 11, 2018

miri64 added a commit to miri64/RIOT that referenced this pull request Sep 11, 2018

Merge 'pr/make/jerryscript/clean' (RIOT-OS#9824)

d1051ad

miri64 mentioned this pull request Sep 11, 2018

murdock: add selected cortex-m boards to LLVM build #9809

Merged

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 11, 2018

smlng reviewed Sep 25, 2018

View reviewed changes

cladmi modified the milestones: Release 2018.10, Release 2019.01 Oct 21, 2018

cladmi added 4 commits December 19, 2018 14:23

pkg/jerryscript: do not use Makefile.base

b7aa55b

Do not include Makefile.base as 'DIRS' are not used here. Also if 'all' target was to be used, there would be two rules creating `$(BINDIR)/jerryscript.a`. By default `Makefile.jerryscript` executes `libjerry` as it is the first target.

pkg/jerryscript: update rules

da3c075

Remove unknown rules from .PHONY. Make 'all' be the default target for jerryscript and execute `all` target. This way no definition order issue could happen in the future.

pkg/jerryscript: use 'make -f' instead of copying makefile

c4f8c00

pkg/jerryscript: remove unnecessary export and fix indent

d55ae9c

cladmi added 2 commits December 19, 2018 14:30

pkg/jerryscript: build directory does not need to exist with -B

0975751

pkg/jerryscript: build into '$(BINDIR)'

3a125b6

Build into the BINDIR directory instead of the source repository. This makes 'clean' work as expected without other intervention. Also it goes in the direction of having the package source repository board independent.

cladmi force-pushed the pr/make/jerryscript/clean branch from babed28 to 3a125b6 Compare December 19, 2018 13:33

aabadie approved these changes Dec 19, 2018

View reviewed changes

aabadie merged commit 8e4b6ed into RIOT-OS:master Dec 19, 2018

cladmi deleted the pr/make/jerryscript/clean branch December 19, 2018 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/jerryscript: build into '$(BINDIR)' and cleanups#9824

pkg/jerryscript: build into '$(BINDIR)' and cleanups#9824
aabadie merged 6 commits intoRIOT-OS:masterfrom
cladmi:pr/make/jerryscript/clean

cladmi commented Aug 22, 2018

Uh oh!

cladmi commented Aug 22, 2018

Uh oh!

cladmi commented Aug 23, 2018 •

edited

Loading

Uh oh!

cladmi commented Aug 23, 2018

Uh oh!

cladmi commented Aug 24, 2018

Uh oh!

smlng Sep 25, 2018

Uh oh!

cladmi Sep 25, 2018 •

edited

Loading

Uh oh!

smlng Sep 25, 2018

Uh oh!

cladmi Sep 25, 2018

Uh oh!

aabadie commented Dec 19, 2018

Uh oh!

cladmi commented Dec 19, 2018 •

edited

Loading

Uh oh!

aabadie left a comment

Uh oh!

cladmi commented Dec 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		@@ -1,15 +1,14 @@
		BUILD_DIR ?= $(CURDIR)/riot
		BUILD_DIR ?= $(BINDIR)/jerryscript

Conversation

cladmi commented Aug 22, 2018

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

cladmi commented Aug 22, 2018

Uh oh!

cladmi commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cladmi commented Aug 23, 2018

Uh oh!

cladmi commented Aug 24, 2018

Uh oh!

smlng Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

cladmi Sep 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smlng Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

cladmi Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

aabadie commented Dec 19, 2018

Uh oh!

cladmi commented Dec 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

cladmi commented Dec 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cladmi commented Aug 23, 2018 •

edited

Loading

cladmi Sep 25, 2018 •

edited

Loading

cladmi commented Dec 19, 2018 •

edited

Loading