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

Make install cleaning and improvements #8428

Merged
merged 2 commits into from
Oct 19, 2014

Conversation

nalimilan
Copy link
Member

The first commit makes the tree resulting from make install cleaner.

The second one is an attempt at fixing #8367, but it currently doesn't work. I'd appreciate some help from the genius who wrote Makefile:27. It should be adapted so that the symlink to doc/ is no longer created at $(build_datarootdir)/julia/doc, but at $(build_docdir) -- Make.inc:102 should then also be made consistent with Make.inc:89.

@nalimilan
Copy link
Member Author

@staticfloat Any ideas?

@nalimilan
Copy link
Member Author

@staticfloat I'd also prefer this solution, but currently Makefile:27 makes a mere symlink to the source dir, which saves some space. I guess I could stop doing that if you don't care.

Note to myself: this PR will have to adapt #8466 if examples are not under $(datarootdir)/julia anymore.

@tkelman
Copy link
Contributor

tkelman commented Sep 25, 2014

@nalimilan as will test/examples.jl itself. Currently at least the check in test/runtests.jl and the include paths in test/examples.jl are consistent. If you tweak things to be a little more particular wrt installed location of the examples, make sure the two files stay consistent.

@staticfloat
Copy link
Member

@nalimilan Let's not symlink the doc directory anymore. Let's just copy in what we need.

@jiahao jiahao force-pushed the master branch 3 times, most recently from 6c7c7e3 to 1a4c02f Compare October 11, 2014 22:06
Perf suite contains many files with dubious licensing and is
typically not useful in normal use. Since ed38447 it's no longer
needed to run tests.
@nalimilan
Copy link
Member Author

I think this new version should be fine. Now doc and examples are copied instead of symlinked, and we no longer install unwanted files that we need to remove later.

But @tkelman and @staticfloat, please check the new rules for $(build_docdir) at the top of Makefile: I adapted it from the symlink_target macro, removing what didn't seem to be required: in particular, I didn't understand what the calls to abspath where here for; nor the calls to @-cmd //C rmdir on Windows, since in other places a simple rm -fr is used.

@@ -96,6 +99,9 @@ build_libdir = $(build_prefix)/lib
build_private_libdir = $(build_prefix)/lib/julia
build_libexecdir = $(build_prefix)/libexec
build_datarootdir = $(build_prefix)/share
build_docdir = $(build_datarootdir)/doc/julia/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will cause a problem, but there's a slash at the end of build_docdir that isn't anywhere else, and that SOMETIMES causes a change in behavior for file copying programs like rsync, so it's probably best to keep it consistent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, will fix.

@staticfloat
Copy link
Member

Overall, I really like this change. There's a little bit of mystery surrounding some of the makefile lines, but other than that, looks really good on my end. I'm building it locally just as a sanity check, and will let you know if anything breaks.

Also change the default path for documentation to something more standard.
Copy doc/ and examples/ under $(build_docdir) instead of creating symlinks
as before, so that we don't need to remove unwanted files afterwards.
Finally, fix code relying on old paths.
@tkelman
Copy link
Contributor

tkelman commented Oct 19, 2014

I'm guessing that @-cmd //C rmdir may have been since some versions of rm -fr might interact badly with NTFS junctions, potentially doing bad things like recursing into the target of the link and deleting the originals... That's a guess though. If we're not symlinking to the docs any more, then there's probably no need to worry about it.

@nalimilan
Copy link
Member Author

@tkelman OK, what's more or less what I suspected.

I've updated the branch with the two fixes to issues spotted by @staticfloat above. It should be OK to merge,
unless we want to sort out more deeply the question of absolute paths.

staticfloat added a commit that referenced this pull request Oct 19, 2014
Make install cleaning and improvements
@staticfloat staticfloat merged commit 17311a9 into JuliaLang:master Oct 19, 2014
@waTeim
Copy link
Contributor

waTeim commented Feb 22, 2015

Hey no problem! Confirmed functional after merge on OS/X, Linux, Windows. Now on to #8757; I'll return to that discussion to ask questions.

@waTeim
Copy link
Contributor

waTeim commented Feb 22, 2015

Hey! wrong thread.

@tkelman
Copy link
Contributor

tkelman commented Mar 2, 2015

Hm, we never backported this. Should we? It would help with some conflicts in #10253. Also #8398 would be necessary.

cc @JuliaBackports

@staticfloat
Copy link
Member

I don't think this is urgent enough to backport; it's mostly stuff to make things more "correct", but doesn't fix any big bugs that I can tell.

@tkelman
Copy link
Contributor

tkelman commented Mar 2, 2015

No urgency, not really fixing a bug, but does reduce the number of fiddly conflicts if we want to backport #10253. I could probably limit that to just 94334bc for consistent naming on the buildbots though, and not backport the rest of it.

@vtjnash
Copy link
Member

vtjnash commented Mar 14, 2015

i just got bit by this commit on master, since the files never get recopied from examples when they get modified (i jump around a lot on various branches). this either needs to be reverted to a symlink or written with proper makefile dependency update rules.

@waTeim
Copy link
Contributor

waTeim commented Mar 15, 2015

Oh! is it a possible “julia-config to the rescue” moment?

On Mar 14, 2015, at 5:42 PM, Jameson Nash [email protected] wrote:

i just got bit by this commit on master, since the files never get recopied from examples when they get modified. this either needs to be reverted to a symlink or written with proper makefile dependency update rules.


Reply to this email directly or view it on GitHub.

@tkelman
Copy link
Contributor

tkelman commented Mar 15, 2015

No, it's a "makefiles should not be written by humans" moment since no one ever seems to be able to write them perfectly.

@nalimilan
Copy link
Member Author

Makefiles are also particularly hard to test... As can be seen from the fact that nobody had noticed this bug for months. :-/

Ideally, $(build_docdir) should recursively depend on doc/ (and thus on examples/), so that it's copied again on every change. But it doesn't seem easy in make. We could just mark this as a PHONY target, but running it on every build would be silly. Any ideas?

@nalimilan
Copy link
Member Author

Bump!

@tkelman
Copy link
Contributor

tkelman commented Mar 23, 2015

would the directory timestamp be a decent proxy?

@nalimilan
Copy link
Member Author

Not AFAICT. Switching between two branches which have differences under doc/ does not update the timestamp. You need to create/delete a file for that (and, I think, only at the toplevel). That's really lousy.

@nalimilan
Copy link
Member Author

@staticfloat Do you have any ideas?

@nalimilan nalimilan deleted the makeinstall branch April 6, 2015 10:29
@staticfloat
Copy link
Member

Take a look at this branch and let me know if it fixes it for you.

@nalimilan
Copy link
Member Author

@vtjnash I think you're the best person to check whether @staticfloat's fix works.

@vtjnash
Copy link
Member

vtjnash commented May 23, 2015

Wildcard is preferable to shell find, but you can't use directory timestamps for this, so it wouldn't be entirely reliably. A better option would be to put a dummy file in there and "touch" it at the end of the makefile rule.

@staticfloat
Copy link
Member

Yes, the nice thing about $(shell find examples) is that it recurses.

I'm not sure how the dummy file would work? We want to re-rerun this command every time a file in examples changes, so how would the timestamp of the dummy file get updated when I edit e.g. examples/clustermanager/0mq/head.jl?

@vtjnash
Copy link
Member

vtjnash commented May 23, 2015

Good point about the recursion. The dummy file would be the target, since the timestamp of a folder target is unreliable.

@staticfloat
Copy link
Member

What I mean is how does the timestamp of the dummy file get updated? As it stands, because $(shell find examples) explicitly lists out all the files contained within examples/, we have direct dependency on each file within examples/ and aren't relying on the directory timestamps at all. I suppose we could ask find to only list files and not include directories, but I'm not sure that would help anything.

@vtjnash
Copy link
Member

vtjnash commented May 23, 2015

touch $@

@waTeim
Copy link
Contributor

waTeim commented May 23, 2015

Following on, If you wanted to go down the find-touch route, then something like this might work:

find . -type f -exec touch {} \;

Or some-such. Sorry I have not been paying attention to this thread as much as I should.

@staticfloat
Copy link
Member

I don't think we're talking about the same thing; we want to re-run these commands whenever an external program modifies the files; we don't want to change the timestamps from within the Makefile.

vtjnash added a commit that referenced this pull request Aug 10, 2015
vtjnash added a commit that referenced this pull request Aug 10, 2015
vtjnash added a commit that referenced this pull request Aug 10, 2015
ref #8428 (comments). fix #12539

(cherry-picked from 6c7677c)
vtjnash added a commit that referenced this pull request Aug 16, 2015
vtjnash added a commit that referenced this pull request Sep 11, 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.

6 participants