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' fixes to allow building RPM nightlies again #19539

Merged
merged 3 commits into from
Dec 16, 2016
Merged

Conversation

nalimilan
Copy link
Member

@nalimilan nalimilan commented Dec 8, 2016

The first commit fixes make install with a custom build_bindir. The second one avoids building docs when they are already present (like in tarballs), to avoid requiring an Internet connexion (not available for good reasons in RPM build machines).

This fixes failures introduced by #18588.

@nalimilan nalimilan mentioned this pull request Dec 8, 2016
4 tasks
@kshyatt kshyatt added the building Build system, or building Julia or its dependencies label Dec 10, 2016
@nalimilan nalimilan changed the title Fix 'make install' with custom build_bindir Two fixes to allow building RPM nightlies again Dec 10, 2016
@nalimilan nalimilan changed the title Two fixes to allow building RPM nightlies again Two 'make install' fixes to allow building RPM nightlies again Dec 10, 2016
@@ -5,7 +5,7 @@ default: html
# You can set these variables from the command line.
SRCDIR := $(abspath $(dir $(lastword $(MAKEFILE_LIST))))
JULIAHOME := $(abspath $(SRCDIR)/..)
JULIA_EXECUTABLE := $(JULIAHOME)/usr/bin/julia
JULIA_EXECUTABLE := $(build_bindir)/julia
Copy link
Contributor

Choose a reason for hiding this comment

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

build_bindir won't usually be defined here

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, indeed. I've changed the commit to include Make.inc, just like e.g. src/Makefile.

@@ -316,9 +316,8 @@ define stringreplace
$(build_depsbindir)/stringreplace $$(strings -t x - $1 | grep '$2' | awk '{print $$1;}') '$3' 255 "$(call cygpath_w,$1)"
endef

install: $(build_depsbindir)/stringreplace
install: $(build_depsbindir)/stringreplace $(BUILDROOT)/doc/_build/html
Copy link
Contributor

Choose a reason for hiding this comment

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

we should try to ensure somehow that this is up to date, so we don't use a stale copy

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we could list all files under base and doc as dependencies so that make compares their timestamps to that of the html directory. Looks like this would do the trick. How does that sound?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a commit doing that.

'make install' failed when setting build_bindir since docs/Makefile expected
the julia executable to be under $(JULIAHOME)/usr/bin/.
There's not point in rebuilding docs if they are up-to-date, and this
allows building tarballs (which already include HTML docs) without Internet
access.
@nalimilan nalimilan changed the title Two 'make install' fixes to allow building RPM nightlies again 'make install' fixes to allow building RPM nightlies again Dec 10, 2016
@nalimilan
Copy link
Member Author

Good to merge now?

@@ -5,7 +5,8 @@ default: html
# You can set these variables from the command line.
SRCDIR := $(abspath $(dir $(lastword $(MAKEFILE_LIST))))
JULIAHOME := $(abspath $(SRCDIR)/..)
JULIA_EXECUTABLE := $(JULIAHOME)/usr/bin/julia
include $(JULIAHOME)/Make.inc
Copy link
Contributor

Choose a reason for hiding this comment

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

do you run this often in a context where you couldn't set JULIA_EXECUTABLE to reflect the custom bindir?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably not, but I couldn't find a drawback to doing things that way. It's also more consistent with other Makefiles. I can remove it if you like.

Copy link
Contributor

Choose a reason for hiding this comment

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

presumably there's some reason it wasn't doing this include before?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hard to tell. Anyway, as I said, I can remove it if you want.

Copy link
Contributor

@tkelman tkelman Dec 16, 2016

Choose a reason for hiding this comment

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

I'm coming up with some workarounds for the windows buildbot issues where it turns out it would be useful to have included Make.inc in this file, so merging.

@nalimilan
Copy link
Member Author

I'll merge tomorrow barring objections.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
building Build system, or building Julia or its dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants