depends: sed robustness #16838

1. 
   dongcarl commented at 7:17 pm on September 9, 2019: contributor

   As seen in #16837, the sed calls in depends fail silently, and can result in silent, unwanted regressions when packages are updated.

   It seems that a possible solution might be something like the following: https://askubuntu.com/a/1036918

2. dongcarl added the label Build system on Sep 9, 2019
3. dongcarl added the label good first issue on Sep 9, 2019
4. 
   laanwj commented at 7:36 am on September 10, 2019: member

   +1 for improving robustness here. Silently failing errors are horrible for troubleshooting.

   Edit: #16837 (review) illustrates, again, why this is important

5. 
   fanquake commented at 5:16 am on September 16, 2019: member

   I spent some time looking at this today. Would it be ok if we required gnu sed in depends (there is some precedent as we require glibtoolize elsewhere in the build system)? Otherwise I’m not sure that the suggested approach would work out of the box on macOS (BSD sed).

   I did have a look at the approach in https://stackoverflow.com/a/28966696, but that seemed a bit messy. Will revisit later on.

6. 
   laanwj commented at 10:36 am on September 16, 2019: member

   Doesn’t sed -i already rely on GNU sed? I don’t remember seeing it in BSDs.

   Apparently it does, just checked on FreeBSD at least and

   0     -i extension
   1             Edit files in-place similarly to -I, but treat each file
   2             independently from other files.  In particular, line numbers in
   3             each file start at 1, the "$" address matches the last line of
   4             the current file, and address ranges are limited to the current
   5             file.  (See Sed Addresses.) The net result is as though each file
   6             were edited by a separate sed instance.
   

7. 
   MarcoFalke commented at 4:18 pm on September 16, 2019: member

   Couldn’t we mock (or wrap) sed while doing depends? That way we can control the exit code.

   A wrapper that does the following, should be good enough:

   0  - check that ${file}_BACKUP does not exist
   1  - run replacement with unmodified sed on $file, additionally passing `--in-place=_BACKUP`
   2  - assert that $file differs from $file_BACKUP
   3  - delete $file_BACKUP
   

8. 
   dongcarl commented at 5:38 pm on September 16, 2019: contributor

   Couldn’t we mock (or wrap) sed while doing depends? That way we can control the exit code.

   A wrapper that does the following, should be good enough:

   0  - check that ${file}_BACKUP does not exist
   1  - run replacement with unmodified sed on $file, additionally passing `--in-place=_BACKUP`
   2  - assert that $file differs from $file_BACKUP
   3  - delete $file_BACKUP
   

   How would we make that wrapper accessible in the Makefile? Somethine like SHELL = /usr/bin/env PATH=$(CURDIR)/wrappers:${PATH} sh?

9. 
   MarcoFalke commented at 7:14 pm on September 16, 2019: member

   Yeah, something like that.
10. fanquake referenced this in commit cc1d7fd57c on Sep 18, 2019
11. sidhujag referenced this in commit ccbf64836d on Sep 23, 2019
12. laanwj referenced this in commit 003f2d20b1 on Sep 26, 2019
13. sidhujag referenced this in commit 2cf7e753ae on Sep 26, 2019
14. fanquake referenced this in commit 2562d5d238 on Aug 27, 2020
15. sidhujag referenced this in commit a012bea252 on Aug 28, 2020
16. UdjinM6 referenced this in commit bd311c3654 on Dec 18, 2020
17. fanquake removed the label good first issue on Mar 31, 2021
18. 
    fanquake commented at 8:48 am on March 31, 2021: member

    I’ve removed “good first issue” here, as the remaining sed usage is basically contained to qt (1 in libxcb that we ), and is non-trivial to remove.
19. PastaPastaPasta referenced this in commit cad871801d on Jun 27, 2021
20. PastaPastaPasta referenced this in commit 512b60afdc on Jun 28, 2021
21. PastaPastaPasta referenced this in commit eaa3781020 on Jun 29, 2021
22. PastaPastaPasta referenced this in commit 3a73302be0 on Jul 1, 2021
23. PastaPastaPasta referenced this in commit 8424bfff25 on Jul 1, 2021
24. PastaPastaPasta referenced this in commit 768c1d46e0 on Jul 12, 2021
25. PastaPastaPasta referenced this in commit 6638fa763c on Jul 13, 2021
26. gades referenced this in commit 3d84f0b08c on Apr 20, 2022
27. fanquake closed this on Jul 30, 2022

    ---

28. sidhujag referenced this in commit 1cdef88224 on Aug 1, 2022
29. bitcoin locked this on Jul 30, 2023

Contributors
dongcarl laanwj fanquake MarcoFalke

Labels
Build system