#26298 closed defect (fixed)

Add build/bin to path in bootstrap script

Reported by: embray Owned by:
Priority: blocker Milestone: sage-8.4
Component: build Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: 113a14f (Commits) Commit: 113a14f1e29da64da9b9c6b59526e8cd5e2c5ce4
Dependencies: Stopgaps:

Description

bootstrap runs build/bin/sage-download-file, but since #18438 this has sage-system-python in its shebang line, which is not in the path by default.

This can be fixed by putting $SAGE_ROOT/build/bin explicitly on the $PATH.

Change History (22)

comment:1 Changed 22 months ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/ticket-26298
  • Commit set to 4652769a66efde34b7e9c66f474e1f24ad96c666
  • Status changed from new to needs_review

New commits:

4652769Trac #26298: always put $SAGE_ROOT/build/bin on the PATH for bootstrap

comment:2 Changed 22 months ago by vbraun

Bootstrap still fails; I force download with

$ git clean -f -d -x
$ ./bootstrap -D
rm -rf config configure build/make/Makefile-auto.in
./bootstrap: line 61: sage-download-file: command not found
Error: downloading configure-282.tar.gz failed

The problem is that SAGE_ROOT isn't actually set to anything...

See also https://groups.google.com/d/msg/sage-devel/iCrymoWbWxo/AOeOKFEpCwAJ

comment:3 Changed 22 months ago by embray

  • Status changed from needs_review to needs_work

D'oh, that is true. I guess if SAGE_ROOT is not already set it should set SAGE_ROOT to the directory the script is in.

comment:4 Changed 22 months ago by embray

It occurs to me that at some point I exported SAGE_ROOT to my environment, so it still worked.

comment:5 Changed 22 months ago by embray

Hmm, no, that doesn't seem to be it either. Even if I make sure SAGE_ROOT is not set and PATH is also clean, ./bootstrap -D still works for me on this branch.

comment:6 Changed 22 months ago by embray

I see, ./bootstrap does eventually source src/bin/sage-env. So why would it not work?

comment:7 Changed 22 months ago by embray

Oh, I guess at this point src/bin/sage-env-config is not generated yet, so you also have to make sure that is deleted.

comment:8 Changed 22 months ago by git

  • Commit changed from 4652769a66efde34b7e9c66f474e1f24ad96c666 to 349dc06a6ce6a45dcc597f04810155d70b773a7f

Branch pushed to git repo; I updated commit sha1. New commits:

349dc06First make sure SAGE_ROOT is set, if not already, to the path to the bootstrap script.

comment:9 Changed 22 months ago by embray

  • Status changed from needs_work to needs_review

comment:10 Changed 22 months ago by embray

I'm still not wild about this, but I'm open to other ideas. Maybe there's a simpler solution I'm not seeing.

comment:11 follow-up: Changed 22 months ago by mkoeppe

Looks good to me, but perhaps it would make sense to simplify it by unconditionally setting SAGE_ROOT to the path of the script.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 22 months ago by embray

Replying to mkoeppe:

Looks good to me, but perhaps it would make sense to simplify it by unconditionally setting SAGE_ROOT to the path of the script.

I'm inclined to agree, but I'm not positive either. Is there any one would ever want to set SAGE_ROOT to a different path here, e.g. with mind towards building in a separate directory from source (e.g. VPATH style, although with VPATH builds I think it still assumes configure is in the source directory).

comment:13 in reply to: ↑ 12 Changed 22 months ago by mkoeppe

Replying to embray:

Is there any one would ever want to set SAGE_ROOT to a different path here, e.g. with mind towards building in a separate directory from source

I think it's an ancient, long obsolete use-case. Note that configure unconditionally sets SAGE_ROOT, so let's match that.

(e.g. VPATH style, although with VPATH builds I think it still assumes configure is in the source directory).

definitely don't need for VPATH.

comment:14 Changed 22 months ago by embray

Ok, I'll make that simplification then.

comment:15 Changed 22 months ago by git

  • Commit changed from 349dc06a6ce6a45dcc597f04810155d70b773a7f to 113a14f1e29da64da9b9c6b59526e8cd5e2c5ce4

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

113a14fFirst make sure SAGE_ROOT is set, if not already, to the path to the bootstrap script.

comment:16 Changed 22 months ago by embray

Works for me, but someone else should confirm.

comment:17 Changed 22 months ago by mkoeppe

The commit message should probably be adjusted.

comment:18 Changed 22 months ago by mkoeppe

Otherwise looks good to me and seems to work.

comment:19 Changed 22 months ago by mkoeppe

  • Reviewers set to Matthias Koeppe
  • Status changed from needs_review to positive_review

comment:20 Changed 22 months ago by embray

I agree the commit message should be updated. I'm not sure if it's too late or not though.

comment:21 Changed 22 months ago by embray

I'll go ahead and give it a try.

comment:22 Changed 22 months ago by vbraun

  • Branch changed from u/embray/ticket-26298 to 113a14f1e29da64da9b9c6b59526e8cd5e2c5ce4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.