Opened 6 years ago

Closed 6 years ago

#19266 closed enhancement (fixed)

configure: create directories using AC_CONFIG_COMMANDS

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.9
Component: build Keywords:
Cc: tscrim Merged in:
Authors: Jeroen Demeyer Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: c128c9d (Commits, GitHub, GitLab) Commit: c128c9dff66978fb67b255988f23aacf5bdc7bac
Dependencies: Stopgaps:

Status badges

Change History (16)

comment:1 Changed 6 years ago by jdemeyer

  • Branch set to u/jdemeyer/config_status_mkdir

comment:2 Changed 6 years ago by jdemeyer

  • Cc tscrim added
  • Commit set to 9861f6051b40fa7fbb601113c01db2adcc37cf76
  • Status changed from new to needs_review

New commits:

dcf0098Move configuration part of build/make/install to configure
f708487Fix for Debian/Ubuntu GCC version numbers
52a3cf0Add rules for installing packages with pip
919da67Merge #19187 and #18885 on top of 6.9.beta7
dea2513Temporary roll back changes between 6.9.beta2 and 6.9.beta8
0c60518Merge 6.9.beta8 except build/make/install into t/19043/move_install_to_configure
4178b80Move changes of build/make/install to configure.ac
9861f60Create directories using AC_CONFIG_COMMANDS

comment:3 follow-up: Changed 6 years ago by tscrim

Is there a particular reason why you removed the SAGE_LOGS assignment and not the others? I would think this environment variable is still needed to be set for

echo >&7 " sage-logger 'sage --pip install $PKG_NAME' \$(SAGE_LOGS)/$PKG_NAME.log"

Or is it only okay because it will be set by configure and then only run in the generated makefile (and the others are used by configure)?

comment:4 in reply to: ↑ 3 ; follow-up: Changed 6 years ago by jdemeyer

Replying to tscrim:

Is there a particular reason why you removed the SAGE_LOGS assignment and not the others?

Just because $SAGE_LOGS isn't used.

I would think this environment variable is still needed to be set for

echo >&7 " sage-logger 'sage --pip install $PKG_NAME' \$(SAGE_LOGS)/$PKG_NAME.log"

You are mis-reading that. That command is writing the literal string $(SAGE_LOGS), not the current value of SAGE_LOGS.

comment:5 in reply to: ↑ 4 Changed 6 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Replying to jdemeyer:

Replying to tscrim:

I would think this environment variable is still needed to be set for

echo >&7 " sage-logger 'sage --pip install $PKG_NAME' \$(SAGE_LOGS)/$PKG_NAME.log"

You are mis-reading that. That command is writing the literal string $(SAGE_LOGS), not the current value of SAGE_LOGS.

Ah, yes. :P LGTM (modulo a sage --fix-pkg-checksums with the latest version from #19043).

comment:6 Changed 6 years ago by jdemeyer

  • Status changed from positive_review to needs_work

comment:7 Changed 6 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:8 follow-up: Changed 6 years ago by git

  • Commit changed from 9861f6051b40fa7fbb601113c01db2adcc37cf76 to d12c0a011d865fb0ede0ad98fd2dbde5f157dd4d

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

b45aad3Replace echo -n by AS_ECHO_N
d12c0a0Bump configure version

comment:9 in reply to: ↑ 8 Changed 6 years ago by jdemeyer

I discovered one more issue: echo -n is not very portable, it should be replaced by AS_ECHO_N.

Last edited 6 years ago by jdemeyer (previous) (diff)

comment:10 Changed 6 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:11 Changed 6 years ago by jdemeyer

Sorry, not ready yet: there are more instances of echo -n.

Last edited 6 years ago by jdemeyer (previous) (diff)

comment:12 follow-up: Changed 6 years ago by git

  • Commit changed from d12c0a011d865fb0ede0ad98fd2dbde5f157dd4d to c128c9dff66978fb67b255988f23aacf5bdc7bac

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

1570875Replace echo -n by AS_ECHO_N
c128c9dBump configure version

comment:13 in reply to: ↑ 12 Changed 6 years ago by jdemeyer

  • Status changed from needs_work to needs_review

Can you please review this commit:

1570875Replace echo -n by AS_ECHO_N

And this is just for Volker's buildbot:

c128c9dBump configure version

comment:14 Changed 6 years ago by vbraun

  • Dependencies #19043 deleted

comment:15 Changed 6 years ago by tscrim

  • Status changed from needs_review to positive_review

LGTM.

comment:16 Changed 6 years ago by vbraun

  • Branch changed from u/jdemeyer/config_status_mkdir to c128c9dff66978fb67b255988f23aacf5bdc7bac
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.