bugGNU Octave - Bugs: bug #60413, build: sed expression problems...

 
 

bug #60413: build: sed expression problems when prefix is empty or slash

Submitter:  Mike Miller <mtmiller>
Submitted:  Sun 18 Apr 2021 12:25:30 AM UTC
   
 
Category:  Configuration and Build System Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Unexpected Error or Warning
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 28 Apr 2021 09:27:19 PM UTC, comment #24: 

I meant to state the ${} syntax.  The space will delimit the name so there is no ambiguity in interpretation as Mike mentioned.

Question is whether we also want consistency.  For example,


mk-version-h.in.sh:  -e "s|%OCTAVE_VERSION%|\"${OCTAVE_VERSION}\"|"


We could write just "$OCTAVE_VERSION" here, right?  It's fine with me any of the three ways: 1) $SED, $OCTAVE_VERSION, 2) ${SED}, ${OCTAVE_VERSION}, or 3) $SED, ${OCTAVE_VERSION}.

And to answer jwe's question, one needs to use quotes if the variable expands to include a space.  So, "$SED" or "${SED}".


  It's fine with me either way.  The space will delimit the name.

Rik <rik5>
Group administrator
Wed 28 Apr 2021 08:45:13 PM UTC, comment #23: 

From the bash manual:

> The braces are required when parameter is a positional parameter with more than one digit, or when parameter is followed by a character that is not to be interpreted as part of its name.


I almost never use braces in shell scripts when a variable is clearly separated from surrounding text by spaces or punctuation.

Mike Miller <mtmiller>
Group Member
Wed 28 Apr 2021 08:33:21 PM UTC, comment #22: 

Rik: The .mk files are different, they are Makefile fragments where the parens are required, otherwise $PERL is equivalent to ${P}ERL.  In a shell script, normal parens $(PERL) will be evaluated as a shell command substitution and execute PERL, stuffing any output back into the shell script expression where $(PERL) is used.  Maybe you are thinking of ${PERL} instead of just $PERL in shell scripts?  I don't see that we've been consistent with that either.

Where it might matter is when the command is a full file name that contains spaces.  Does ${} protect that, or is "${}" quoting needed?  Either way I don't think we've ever done the work so that the build system can work properly in a directory tree that has spaces in the name.

John W. Eaton <jwe>
Group administrator
Wed 28 Apr 2021 08:17:21 PM UTC, comment #21: 

I don't think it's super critical, but we've used the explicit form of variable substitution in other parts of Octave.  For example, if you look in doc/interpreter/module.mk you will find


        $(PERL) $(srcdir)/%reldir%/munge-texi.pl $(top_srcdir) $(DOCSTRING_FILES) < $< > $@-t && \


This could have been written "$PERL", but the extra parentheses really make it clear that we are doing variable substitution.

In this case, we could write "$(SED)", but "$SED" is probably fine too.

Rik <rik5>
Group administrator
Wed 28 Apr 2021 06:12:55 PM UTC, comment #20: 
Markus Mützel <mmuetzel>
Group administrator
Tue 27 Apr 2021 04:21:00 PM UTC, comment #19: 

The buildbots seem to be fine with this.

Closing as fixed.

Markus Mützel <mmuetzel>
Group administrator
Mon 26 Apr 2021 06:24:27 PM UTC, comment #18: 

Thanks for testing.

I added the new configure flag in the build rules for Octave in MXE Octave here:
https://hg.octave.org/mxe-octave/rev/04228fb2b15a

The buildbots failed this cycle (for a different reason). I'd like to wait and see if they are ok with these changes before closing this as fixed.

Markus Mützel <mmuetzel>
Group administrator
Mon 26 Apr 2021 06:20:16 PM UTC, comment #17: 

Thanks, the nightly snap build looks much better. No sed errors in the build log and the *FLAGS variables are no longer empty.

If it works for you when you do want to enable this for Windows builds, you can call this fixed.

Mike Miller <mtmiller>
Group Member
Sun 25 Apr 2021 03:10:43 PM UTC, comment #16: 

Ok. I pushed the patch to stable here:
https://hg.savannah.gnu.org/hgweb/octave/rev/99e3912441ec

And merged - with what I think were the equivalent changes - on the default branch here:
https://hg.savannah.gnu.org/hgweb/octave/rev/0f57f0f47216

Marking as ready for test.

Markus Mützel <mmuetzel>
Group administrator
Sat 24 Apr 2021 07:42:59 PM UTC, comment #15: 

Sure, the patch looks ok to me, not yet tested.

By the way, it's not preventing snap packaging, it's more of an annoying error message in the build logs and bogus strings in the resulting build flags in mkoctfile. But I'm working around those with custom edits in some cases anyway, and can easily add more to ensure users get the right values.

Mike Miller <mtmiller>
Group Member
Sat 24 Apr 2021 05:24:56 PM UTC, comment #14: 

Attached is a patch that adds the configure option "--enable-relocate-all" that enables the prefix replacements in the default mkoctfile flags. By default, that option is disabled.
Imho, it'd probably be best if we enabled it only manually if necessary (i.e. in the MXE Octave build rules) and don't try to "guess" a different default dependent on other settings.

I'm not sure if this should go on the stable branch. I think it probably should because the current behavior seems to prevent packaging Octave in a snap package.

What do you think?


(file #51314)

Markus Mützel <mmuetzel>
Group administrator
Wed 21 Apr 2021 06:52:22 PM UTC, comment #13: 

Yeah, maybe a configure option would be best. But I would only enable it by default when you are cross-compiling specifically on Windows. I can in theory cross-compile Octave on a linux distro for another arch or another distro, and that shouldn't enable these relocations either.

Mike Miller <mtmiller>
Group Member
Mon 19 Apr 2021 06:47:14 AM UTC, comment #12: 

@Mike: Your comment #11 pretty much covers the motivation for the replacement.
I was thinking that on most other platforms the replacements (forth and back) would be a no-op. But I agree that we should probably be more careful with them.

Should we maybe have a configure option that turns these replacements on or off (e.g. "--enable-relocate-all")?
That option could be disabled by default. Maybe we could enable it by default if we detect that we are cross-compiling.

Markus Mützel <mmuetzel>
Group administrator
Sun 18 Apr 2021 09:49:48 PM UTC, comment #11: 

Markus - I'm not completely clear on the use case that this code was added to solve. Is this meant solely for the case where you are building an Octave bundle with all libraries and tools installed into the same ${prefix}, and you then want that entire bundle to be relocatable after installing on a target system?

I think the assumption that Octave's ${prefix} is the same as FFTW's ${prefix} or GCC's ${prefix}, or any other library, definitely isn't universal, if that's what's going on here. It certainly does seem to make sense for the Windows bundle, where you're building and including an entire GNU runtime in the same tree.

I think for most GNU/Linux users, a relocatable Octave would only substitute the directories that have to do with Octave itself, and not touch variables like CPPFLAGS and LDFLAGS, because those external libraries aren't necessarily going to be moving along with Octave.

Mike Miller <mtmiller>
Group Member
Sun 18 Apr 2021 02:43:48 PM UTC, comment #10: 

Octave's configure script checks for a version of sed and tries to use GNU sed if it is available.  These scripts have a substitution for SED.  It's probably just an oversight that they don't use it consistently.

John W. Eaton <jwe>
Group administrator
Sun 18 Apr 2021 11:33:13 AM UTC, comment #9: 

And next iteration to also match the prefix at the beginning of the variable:

if test -n "$prefix"; then echo "$var" | sed -E "s%(-[LI]|[ ,]|^)${prefix}([/\\ ]|$)%\1\$\{prefix\}\2%g"; else echo "$var"; fi


Markus Mützel <mmuetzel>
Group administrator
Sun 18 Apr 2021 11:21:49 AM UTC, comment #8: 

The sed command in comment #7 wouldn't correctly match the prefix in OCT_LINK_OPTS which looks like the following on Windows with Octave 6.2:

>> mkoctfile -p OCT_LINK_OPTS
-Wl,-rpath-link,C:\PROGRA~1\GNUOCT~1\OCTAVE~2.0\mingw64\/lib -LC:\PROGRA~1\GNUOCT~1\OCTAVE~2.0\mingw64\/lib -LC:\PROGRA~1\GNUOCT~1\OCTAVE~2.0\mingw64\/qt5/lib -Wl,--export-all-symbols


Maybe something like this instead?

if test -n "@prefix@"; then echo '@OCT_LINK_OPTS@' | sed -E "s%(-L|-I| |,)@prefix@([/\\ ]|$)%\1\$\{prefix\}\2%g"; else echo '@OCT_LINK_OPTS@'; fi


Still not sure about "sed -E" or "sed -r". Neither seems to be documented for POSIX sed:
https://www.unix.com/man-page/posix/1p/sed/

Markus Mützel <mmuetzel>
Group administrator
Sun 18 Apr 2021 11:01:52 AM UTC, comment #7: 

Along the lines that Mike suggested in comment #0, maybe, we could change the sed command to something like this:

if test -n "$prefix"; then echo "$var" | sed -E "s%(-L|-I| )${prefix}([/\\ ]|$)%\1\$\{prefix\}\2%g"; else echo "$var"; fi


IIUC, that would only replace anything in the respective variable ($var in the example) if the prefix is non-empty and the prefix is preceded by "-I", "-L" or " " in the variable and it is followed by "/", "\" or " " or ends with the prefix.

An example in subst-config-vals.in.h could maybe look like:

if test -n "@prefix@"; then echo '@CPPFLAGS@' | sed -E "s%(-L|-I| )@prefix@([/\\ ]|$)%\1\$\{prefix\}\2%g"; else echo '@CPPFLAGS@'; fi


Does "sed -E" work on all platforms we'd like to support (GNU, BSD, Mac, ...)? Fwiw, "sed -r" seems to work similarly for me with GNU sed 4.7 if that should be better supported cross-platform.

Markus Mützel <mmuetzel>
Group administrator
Sun 18 Apr 2021 06:34:34 AM UTC, comment #6: 

IIRC, I added the pattern replacements to fix issues with the mkoctfile flags when cross compiling for Windows. If this is the only use case where having those pattern replacements makes sense, maybe we could use them only in that case.

Markus Mützel <mmuetzel>
Group administrator
Sun 18 Apr 2021 12:56:45 AM UTC, comment #5: 

The script code in question looks like it was added just before the 6.1.0 release last fall, related to bug #59173 (https://hg.savannah.gnu.org/hgweb/octave/rev/d7169f012ee1), so it's a pretty new addition to the build system.

Mike Miller <mtmiller>
Group Member
Sun 18 Apr 2021 12:45:37 AM UTC, comment #4: 
Mike Miller <mtmiller>
Group Member
Sun 18 Apr 2021 12:44:10 AM UTC, comment #3: 

Yes, the null string is supposed to equate to "install this in the root directory".

Well I'm running into this now, because this is exactly how snapcraft builds snap applications. 😅

I didn't see these errors when I first snapped Octave 5.2, but now I started seeing them with 6.2 and with the default branch.

Mike Miller <mtmiller>
Group Member
Sun 18 Apr 2021 12:38:07 AM UTC, comment #2: 

What is the use case for using a null prefix?

Is null supposed to transform automatically to root ('/') according to GNU build standards?

What is the frequency that users that attempt this?  Seems like this is the first report of this issue in 20+ years of Octave's existence.

Rik <rik5>
Group administrator
Sun 18 Apr 2021 12:35:43 AM UTC, comment #1: 

As another odd example showing how these substitutions can go very wrong, build with './configure --prefix=/u', yes a new top-level directory named "/u". And let's add CPPFLAGS=-I/usr/local/include and LDFLAGS=-L/usr/local/lib into the mix.

The resulting libinterp/build-env.cc has:


    const char *CPPFLAGS = "-I${prefix}sr/local/include";
    const char *FLIBS = " -L${prefix}sr/local/lib -L${prefix}sr/lib/gcc/x86_64-linux-gnu/10 -L${prefix}sr/lib/gcc/x86_64-linux-gnu/10/../../../x86_64-linux-gnu -L${prefix}sr/lib/gcc/x86_64-linux-gnu/10/../../../../lib -L/lib/x86_64-linux-gnu -L/lib/../lib -L${prefix}sr/lib/x86_64-linux-gnu -L${prefix}sr/lib/../lib -L${prefix}sr/lib/gcc/x86_64-linux-gnu/10/../../.. -lgfortran -lm -lquadmath";
    const char *LDFLAGS = "-L${prefix}sr/local/lib";


Mike Miller <mtmiller>
Group Member
Sun 18 Apr 2021 12:25:30 AM UTC, original submission:  

When building with './configure --prefix=' (prefix is set to the empty string ""), I get the following sed errors from the subst-config-vals.sh script:


$ make libinterp/build-env.cc
sed: -e expression #1, char 0: no previous regular expression
sed: -e expression #1, char 0: no previous regular expression
sed: -e expression #1, char 0: no previous regular expression
sed: -e expression #1, char 0: no previous regular expression
sed: -e expression #1, char 0: no previous regular expression
sed: -e expression #1, char 0: no previous regular expression


It also sets the resulting CPPFLAGS, FLIBS, and LDFLAGS variables to empty in the generated source files, e.g. libinterp/build-env.cc.

When building with './configure --prefix=/' (prefix is set to the string "/"), I get no sed errors or warnings from the build, but the CPPFLAGS, FLIBS, and LDFLAGS variables are really messsed up in the generated source files. Worst example:


    const char *FLIBS = " -L${prefix}usr${prefix}lib${prefix}gcc${prefix}x86_64-linux-gnu${prefix}10 -L${prefix}usr${prefix}lib${prefix}gcc${prefix}x86_64-linux-gnu${prefix}10${prefix}..${prefix}..${prefix}..${prefix}x86_64-linux-gnu -L${prefix}usr${prefix}lib${prefix}gcc${prefix}x86_64-linux-gnu${prefix}10${prefix}..${prefix}..${prefix}..${prefix}..${prefix}lib -L${prefix}lib${prefix}x86_64-linux-gnu -L${prefix}lib${prefix}..${prefix}lib -L${prefix}usr${prefix}lib${prefix}x86_64-linux-gnu -L${prefix}usr${prefix}lib${prefix}..${prefix}lib -L${prefix}usr${prefix}lib${prefix}gcc${prefix}x86_64-linux-gnu${prefix}10${prefix}..${prefix}..${prefix}.. -lgfortran -lm -lquadmath";


Because of the global substitution, every "/" character has been turned into the string "${prefix}".

Maybe both of these cases could be resolved by looking more specifically for patterns like prefix following a literal "-I", "-L", or a space?

And specifically for the root directory as install prefix, maybe all of these substitutions should be skipped entirely? Maybe condition it on prefix not being empty or not being entirely one or more slashes?

Mike Miller <mtmiller>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #51314:  bug60413-relocate-all.patch added by mmuetzel (5KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-04-27 mmuetzel StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2021-04-25 mmuetzel StatusConfirmed Ready For Test
    2021-04-24 mmuetzel Attached File- Added bug60413-relocate-all.patch, #51314
    2021-04-18 mtmiller StatusNeed Info Confirmed
    2021-04-18 rik5 StatusNone Need Info
    2021-04-18 mtmiller Carbon-CopyRemoved 80942 -

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code