try to bring some sanity to the embedding makefile #21427

vtjnash · 2017-04-18T01:16:38Z

we'll still want to eventually decide how to make julia-config.jl output correct relative rpaths
but this should at least get `make -C embedding' working correctly

[x-ref #21299 and precursors]

tkelman · 2017-04-19T02:36:20Z

I don't think we should be building it by default as a requirement of make install right now.

vtjnash · 2017-04-19T14:10:37Z

Are we installing it or not. This half-wise state has lead to some really awful makefiles. This fixes that so that it works correctly.

tkelman · 2017-04-19T14:38:34Z

I don't think we should do so by default, only upon request.

tkelman · 2017-04-19T14:40:43Z

examples/Makefile


-embedding_binary := $(abspath $(outdir)/embedding$(JULIA_LIBSUFFIX)$(EXE))
+embedding-args := \


underscores would be preferable to dashes in variable names IMO

likewise with CFLAGS_ADD

there shouldn't be anything wrong with using more of the allowed characters

stylistically preferable and more uniform to stick with underscores consistently though

tkelman · 2017-04-19T14:41:33Z

examples/embedding/Makefile

@@ -11,16 +11,19 @@ ifndef BIN
 endif

 #=============================================================================
+SPAWN ?= $1


this needs a comment

tkelman · 2017-04-19T19:37:41Z

contrib/julia-config.jl

-        return windowsInitDir()
-    end
+    dir, bin = splitdir(JULIA_HOME)
+    return joinpath(dir, "include", "julia")


this PR isn't making it worse, but this is incorrect if the layout has been set differently, like in a distro build

tkelman · 2017-04-19T22:55:11Z

appveyor.yml

-      .\bin\julia.exe --check-bounds=yes share\julia\test\runtests.jl libgit2-online download pkg embedding
+  #- cd julia-* && .\bin\julia.exe --check-bounds=yes share\julia\test\runtests.jl all &&
+  #    .\bin\julia.exe --check-bounds=yes share\julia\test\runtests.jl libgit2-online download pkg
+  - usr\bin\embedding 2>NUL | usr\bin\julia usr\share\doc\julia\examples\embedding\embedding-test.jl


don't redirect error output

tkelman · 2017-04-20T17:46:35Z

why are you making test/embedding.jl so much harder to invoke?

vtjnash · 2017-04-20T20:06:06Z

Have you tried to invoke the current embedding test? It sounded to me like Jeff almost went mad trying to figure out how to make it run on just our CI configuration.

tkelman · 2017-04-20T21:36:29Z

Yes, my last commit in that PR got it working.

tkelman · 2017-04-20T23:27:11Z

The version on master is far from perfect, but if you run in the right order, make install then make -C examples then run test/embedding.jl from inside the install tree (either via Base.runtests("embedding") or test/runtests.jl embedding), it works. This version is more complicated and no one will ever remember this or invoke it manually.

tkelman · 2017-04-21T15:05:50Z

examples/Makefile

+release: # default target
+# forward all variables expected by the embedding example
+JULIA:=$(call spawn,$(JULIA_EXECUTABLE))
+BIN:=$(build_bindir)


from past discussion it was agreed it should be put in libexecdir, not bindir

Past discussion (#18688 (comment)) was about not installing it. So irrelevant to this PR (which simply doesn't install broken copies of it, like we do now).

Broken how? Usually we try to make the build tree mostly mirror the install tree, except that the former has some things in it that the latter doesn't. Including this executable for now, but if we were to start installing it in the future, I think the structure would be cleaner if it were put in a build-tree location that matched the install-tree location it would get distributed under. And we've discussed that the install tree location shouldn't be publicly exposing such a generic executable name alongside the julia executable. So for the sake of consistency, the build tree shouldn't put it alongside the julia executable either.

Because we currently can't and don't build or install it in a way that mirrors the install tree (this PR gives you more of that flexibility, although more design work in libjulia is still needed)

It's in libexecdir on master. It's not in build_libexecdir in this PR.

shouldn't have ever been using the libexecdir variable since it isn't even a defined location.

What do you mean? Of course it's defined, $(prefix)/libexec

prefix doesn't have a valid value except when executing the install target

When it was a relative path for no good reason it was nonsense, but now that it isn't? Your unstated rules for "valid" aren't making a whole lot of sense.

I'm am building off your observations on the issues with the original PR (#21299 (comment)):
"It's kind of lying to the makefiles"
"would need changing if you're doing an out-of-tree"
"the embedding executable wouldn't run in a binary install"
Why are you giving me such a hard time about addressing those issues?

With this PR, the "example/embedding" directory is now a complete, independent stand-alone example of how to begin a project with Julia; the example/Makefile is a minimal shim to make it easy to run tests against the example; and the CI configuration includes a directive to test that the example project build works.

Anyways, PR updated to make the embedding example directory independent from the Julia one.

"It's kind of lying to the makefiles"
"would need changing if you're doing an out-of-tree"

those were in response to manually overriding prefix=usr which is no longer happening

"the embedding executable wouldn't run in a binary install"

not fixed yet by this pr, some of this will help get there but refactoring the way the test runs and changing the location the executable gets built aren't needed to do that

Why are you giving me such a hard time about addressing those issues?

You haven't been communicating what is broken that you're fixing, just saying how awful and wrong it is without explanation. And the changes here, for the most part, haven't been addressing the acknowledged issues.

tkelman · 2017-04-22T01:30:49Z

Need to turn back on the rest of the tests, but otherwise cdb160d will do.

tkelman · 2017-04-22T10:39:01Z

contrib/windows/msys_build.sh

@@ -203,7 +203,7 @@ fi
 echo 'FORCE_ASSERTIONS = 1' >> Make.user

 cat Make.user
-make -j3 VERBOSE=1 install
+make -j3 VERBOSE=1


may as well keep the install, it takes an extra couple minutes but we have the capacity and it's worth covering

I thought that this file was supposed to be a general-purpose msys build helper, not specific to appveyor, so I was trying to minimize the testing-specific work it was doing.

"general purpose" is overstating it, but it should be possible to run outside of appveyor. running make install isn't testing specific

we'll still want to eventually decide how to make julia-config.jl output correct relative rpaths but this should at least get `make -C embedding' working correctly

ararslan added building Build system, or building Julia or its dependencies embedding Embedding Julia using the C API labels Apr 18, 2017

ararslan requested a review from tkelman April 18, 2017 04:10

vtjnash force-pushed the jn/embedding-makefile-sanity branch from 3a9a4d6 to 92c09e5 Compare April 18, 2017 22:57

tkelman reviewed Apr 19, 2017

View reviewed changes

vtjnash force-pushed the jn/embedding-makefile-sanity branch from ac6682a to 78f1627 Compare April 19, 2017 20:03

tkelman reviewed Apr 19, 2017

View reviewed changes

tkelman reviewed Apr 21, 2017

View reviewed changes

vtjnash force-pushed the jn/embedding-makefile-sanity branch from 3110140 to cdb160d Compare April 21, 2017 23:46

vtjnash force-pushed the jn/embedding-makefile-sanity branch from cdb160d to 1fb267f Compare April 22, 2017 05:00

tkelman reviewed Apr 22, 2017

View reviewed changes

vtjnash force-pushed the jn/embedding-makefile-sanity branch 2 times, most recently from 05f5c93 to fe0cd38 Compare April 22, 2017 21:42

try to bring some sanity to the embedding makefile

bc90122

we'll still want to eventually decide how to make julia-config.jl output correct relative rpaths but this should at least get `make -C embedding' working correctly

vtjnash force-pushed the jn/embedding-makefile-sanity branch from fe0cd38 to bc90122 Compare April 22, 2017 21:45

vtjnash merged commit 2d59d1e into master Apr 23, 2017

vtjnash deleted the jn/embedding-makefile-sanity branch April 23, 2017 01:42

ihnorton mentioned this pull request Jun 1, 2017

JLOptions.julia_bin incorrect when embedding #18437

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

try to bring some sanity to the embedding makefile #21427

try to bring some sanity to the embedding makefile #21427

vtjnash commented Apr 18, 2017 •

edited by ihnorton

Loading

tkelman commented Apr 19, 2017

vtjnash commented Apr 19, 2017

tkelman commented Apr 19, 2017

tkelman Apr 19, 2017

tkelman Apr 20, 2017

vtjnash Apr 20, 2017

tkelman Apr 20, 2017

tkelman Apr 19, 2017

tkelman Apr 19, 2017

tkelman Apr 19, 2017

tkelman commented Apr 20, 2017

vtjnash commented Apr 20, 2017

tkelman commented Apr 20, 2017

tkelman commented Apr 20, 2017 •

edited

Loading

tkelman Apr 21, 2017

vtjnash Apr 21, 2017

tkelman Apr 21, 2017 •

edited

Loading

vtjnash Apr 21, 2017 •

edited

Loading

tkelman Apr 21, 2017

tkelman Apr 22, 2017

vtjnash Apr 22, 2017

tkelman Apr 22, 2017

vtjnash Apr 22, 2017

tkelman Apr 22, 2017

tkelman commented Apr 22, 2017

tkelman Apr 22, 2017

vtjnash Apr 22, 2017

tkelman Apr 22, 2017


		embedding_binary := $(abspath $(outdir)/embedding$(JULIA_LIBSUFFIX)$(EXE))
		embedding-args := \

try to bring some sanity to the embedding makefile #21427

try to bring some sanity to the embedding makefile #21427

Conversation

vtjnash commented Apr 18, 2017 • edited by ihnorton Loading

tkelman commented Apr 19, 2017

vtjnash commented Apr 19, 2017

tkelman commented Apr 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Apr 20, 2017

vtjnash commented Apr 20, 2017

tkelman commented Apr 20, 2017

tkelman commented Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

vtjnash Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Apr 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash commented Apr 18, 2017 •

edited by ihnorton

Loading

tkelman commented Apr 20, 2017 •

edited

Loading

tkelman Apr 21, 2017 •

edited

Loading

vtjnash Apr 21, 2017 •

edited

Loading