feat(scripts): fix bug about `sed -i` #5169

mask-pp · 2024-02-01T13:03:11Z

Issue Addressed

Which issue # does this PR address?
appear error when run ./setup_time.sh genesis.json:

grep: invalid option -- P
sed: 1: "genesis.json": extra characters at the end of g command
typo: taif in scripts/local_testnet/README.md
The lost of --debug-level flag in bootnode.sh
Avoid using GNU sed/grep in macOS system.

Proposed Changes

Please list or describe the changes introduced by this PR.

Additional Info

Please provide any additional information. For example, future considerations
or information useful for reviewers.

CLAassistant · 2024-02-01T13:03:16Z

All committers have signed the CLA.

michaelsproul · 2024-02-01T21:39:57Z

which OS are you using where it didn't work? AFAIK our scripts are working on Linux and macOS. Are you using a BSD?

mask-pp · 2024-02-02T07:36:35Z

which OS are you using where it didn't work? AFAIK our scripts are working on Linux and macOS. Are you using a BSD?

My OS is macOS m3.

realbigsean · 2024-04-04T15:33:12Z

which OS are you using where it didn't work? AFAIK our scripts are working on Linux and macOS. Are you using a BSD?

On mac you need to install gnu sed and gnu grep to run these scripts. If these changes work for linux an mac out of the box, it'd be nice.

realbigsean · 2024-04-04T15:34:37Z

@mask-pp would you mind signing the CLA?

chong-he

Thanks for the PR, left some comments

chong-he · 2024-04-23T12:42:13Z

scripts/local_testnet/setup_time.sh

@@ -20,16 +20,21 @@ echo "slot_per_epoch=$SLOT_PER_EPOCH"
 genesis_file=$1

 # Update future hardforks time in the EL genesis file based on the CL genesis time
-GENESIS_TIME=$(lcli pretty-ssz --spec $SPEC_PRESET --testnet-dir $TESTNET_DIR BeaconState $TESTNET_DIR/genesis.ssz | jq | grep -Po 'genesis_time": "\K.*\d')
-echo $GENESIS_TIME
+GENESIS_TIME=$(lcli pretty-ssz --spec $SPEC_PRESET --testnet-dir $TESTNET_DIR BeaconState $TESTNET_DIR/genesis.ssz | jq | sed -n 's/.*genesis_time": "\([0-9]*\).*/\1/p')


The original script outputs the genesis_time, so does your script - they do the same thing. Not sure what do you mean by invalid option --P? Does the script not work in your OS (MacOS)?

The original script outputs the genesis_time, so does your script - they do the same thing. Not sure what do you mean by invalid option --P? Does the script not work in your OS (MacOS)?

Yeah, it doesn't work in my MacOS.

scripts/local_testnet/bootnode.sh

chong-he · 2024-04-23T12:49:22Z

scripts/local_testnet/setup.sh

+bootnode_enr=`cat $DATADIR/bootnode/enr.dat`
+echo "- $bootnode_enr" > $TESTNET_DIR/boot_enr.yaml
+
+echo "Generated bootnode enr and written to $TESTNET_DIR/boot_enr.yaml"


Not sure what is the motivation of moving the initialization of boot_node to setup.sh? There is no harm here, just that the original script will have:
Now Generating bootnode enr and
Generated bootnode enr and written to $TESTNET_DIR
written to bootnode.log. Now it is written to setup.log

chong-he · 2024-04-23T12:51:13Z

scripts/local_testnet/setup_time.sh

-echo $CAPELLA_TIME
-sed -i 's/"shanghaiTime".*$/"shanghaiTime": '"$CAPELLA_TIME"',/g' $genesis_file
+echo "SHANGHAI_TIME: $CAPELLA_TIME"
+sed 's/"shanghaiTime".*$/"shanghaiTime": '"$CAPELLA_TIME"',/g' $genesis_file > "/tmp/genesis.json"


Does the command not work in MacOS (I am not familiar with it)?

The original script with option -i in sed is to allow in-file editing straightaway, so it works. The proposed change creates a temporary file to replace the genesis.json, which does the same thing

Does the command not work in MacOS (I am not familiar with it)?

The original script with option -i in sed is to allow in-file editing straightaway, so it works. The proposed change creates a temporary file to replace the genesis.json, which does the same thing

It also doesn't work in MacOS, so I have to change it.

mask-pp · 2024-04-23T14:25:58Z

@mask-pp would you mind signing the CLA?

Done!

mask-pp · 2024-04-23T14:47:12Z

Thanks for the PR, left some comments

Thanks very much for your review man.

chong-he · 2024-04-24T02:03:28Z

Thanks for the PR, left some comments

Thanks very much for your review man.

I am good with the proposed change since it does the same thing, and it will be good for other users using MacOS trying to run a local testnet using the scripts. So I am happy to recommend merging this PR.

Thanks again!

Edit: how about the change in bootnode logs from to setup.sh: #5169 (comment)

It is not a major thing, but may I know the motivation behind? I think that should work on MacOS? If so, we can remain that part the same

mask-pp · 2024-04-24T03:11:34Z

Thanks for the PR, left some comments

Thanks very much for your review man.

I am good with the proposed change since it does the same thing, and it will be good for other users using MacOS trying to run a local testnet using the scripts. So I am happy to recommend merging this PR.

Thanks again!

Edit: how about the change in bootnode logs from to setup.sh: #5169 (comment)

It is not a major thing, but may I know the motivation behind? I think that should work on MacOS? If so, we can remain that part the same

Yeah, the bootnode change is optional. It is correct to write the bootnode log into bootnode.log. In order to reduce unnecessary changes I have reverted it.

chong-he · 2024-04-24T23:38:54Z

Thanks @mask-pp

This seems to be a cleaner solution to enable MacOS to work with the script with sed -i: https://www.reddit.com/r/linux4noobs/comments/ne0ti8/why_does_sed_i_not_work_on_my_mac/

Could you give this a try and see if it works? If so, I can test linux on my end and we can use this cleaner solution

mask-pp · 2024-04-25T02:14:52Z

Thanks @mask-pp

This seems to be a cleaner solution to enable MacOS to work with the script with sed -i: https://www.reddit.com/r/linux4noobs/comments/ne0ti8/why_does_sed_i_not_work_on_my_mac/

Could you give this a try and see if it works? If so, I can test linux on my end and we can use this cleaner solution

It works with sed -i '' in macOS, but it doesn't work in linux.
The best way I can imagine to be compatible with mac and linux is don't use the parameter "-i".

ackintosh

Thanks for this PR!

I also noticed that we need to remove these lines from local-testnet.yml to avoid using GNU sed/grep in CI for macOS:

lighthouse/.github/workflows/local-testnet.yml

Lines 42 to 47 in 8cd2b1c

    
                 - name: Install GNU sed & GNU grep 
        
                   if: matrix.os == 'macos-12' 
        
                   run: | 
        
                     brew install gnu-sed grep 
        
                     echo "$(brew --prefix)/opt/gnu-sed/libexec/gnubin" >> $GITHUB_PATH 
        
                     echo "$(brew --prefix)/opt/grep/libexec/gnubin" >> $GITHUB_PATH

and the README in the local_testnet directory:

lighthouse/scripts/local_testnet/README.md

Line 11 in 003bb0a

    
           MacOS users need to install GNU `sed` and GNU `grep`, and add them both to `PATH` as well.

mask-pp · 2024-04-26T01:29:39Z

Thanks for this PR!

I also noticed that we need to remove these lines from local-testnet.yml to avoid using GNU sed/grep in CI for macOS:

lighthouse/.github/workflows/local-testnet.yml

Lines 42 to 47 in 8cd2b1c

- name: Install GNU sed & GNU grep

if: matrix.os == 'macos-12'

run: |

brew install gnu-sed grep

echo "$(brew --prefix)/opt/gnu-sed/libexec/gnubin" >> $GITHUB_PATH

echo "$(brew --prefix)/opt/grep/libexec/gnubin" >> $GITHUB_PATH

and the README in the local_testnet directory:

lighthouse/scripts/local_testnet/README.md

Line 11 in 003bb0a

MacOS users need to install GNU `sed` and GNU `grep`, and add them both to `PATH` as well.

Deleted.

chong-he · 2024-04-29T12:34:12Z

I am not sure why we need to delete the line on the README.md file? @ackintosh

As for the local-testnet.yml, does #5643 fix it?

ackintosh · 2024-04-30T01:22:04Z

I am not sure why we need to delete the line on the README.md file? @ackintosh

If my understanding of this PR is correct, it eliminates the need for macOS users to install GNU sed/grep. They can now run start_local_testnet.sh using the built-in sed/grep commands. So the line in the README.md that encourages macOS users to install GNU sed/grep can be removed.

ackintosh

Thanks @mask-pp, LGTM.

Could you please update the Proposed Changes section of this comment?

mask-pp · 2024-05-04T01:26:59Z

Thanks @mask-pp, LGTM.

Could you please update the Proposed Changes section of this comment?

changed.

dapplion · 2024-06-27T15:57:56Z

Thank you so much for your contribution @mask-pp, but we recently replaced our local testnet scripts with kurtosis

Replace local testnet script with Kurtosis #5865

mask-pp added 2 commits February 1, 2024 20:56

upgrade scripts

0a283c3

fix bug about scripts

7645a35

michaelsproul added the ready-for-review The code is ready for review label Feb 4, 2024

realbigsean changed the base branch from stable to unstable April 4, 2024 15:26

Merge branch 'unstable' into fix/fix_script_bug

c0954c0

chong-he reviewed Apr 23, 2024

View reviewed changes

mask-pp requested a review from chong-he April 23, 2024 14:47

revert bootnode change

f120e68

mask-pp added 2 commits April 25, 2024 10:18

simplify sed

566c45f

revert sed change

e566c29

ackintosh reviewed Apr 25, 2024

View reviewed changes

fix comments

b16a754

mask-pp requested a review from ackintosh April 26, 2024 01:29

ackintosh approved these changes May 3, 2024

View reviewed changes

dapplion closed this Jun 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(scripts): fix bug about `sed -i` #5169

feat(scripts): fix bug about `sed -i` #5169

mask-pp commented Feb 1, 2024 •

edited

Loading

CLAassistant commented Feb 1, 2024 •

edited

Loading

michaelsproul commented Feb 1, 2024

mask-pp commented Feb 2, 2024

realbigsean commented Apr 4, 2024

realbigsean commented Apr 4, 2024

chong-he left a comment

chong-he Apr 23, 2024

mask-pp Apr 23, 2024 •

edited

Loading

chong-he Apr 23, 2024

chong-he Apr 23, 2024

mask-pp Apr 23, 2024

mask-pp commented Apr 23, 2024

mask-pp commented Apr 23, 2024

chong-he commented Apr 24, 2024 •

edited

Loading

mask-pp commented Apr 24, 2024 •

edited

Loading

chong-he commented Apr 24, 2024

mask-pp commented Apr 25, 2024 •

edited

Loading

ackintosh left a comment

mask-pp commented Apr 26, 2024

chong-he commented Apr 29, 2024

ackintosh commented Apr 30, 2024

ackintosh left a comment

mask-pp commented May 4, 2024

dapplion commented Jun 27, 2024

	- name: Install GNU sed & GNU grep
	if: matrix.os == 'macos-12'
	run: \|
	brew install gnu-sed grep
	echo "$(brew --prefix)/opt/gnu-sed/libexec/gnubin" >> $GITHUB_PATH
	echo "$(brew --prefix)/opt/grep/libexec/gnubin" >> $GITHUB_PATH

feat(scripts): fix bug about sed -i #5169

feat(scripts): fix bug about sed -i #5169

Conversation

mask-pp commented Feb 1, 2024 • edited Loading

Issue Addressed

Proposed Changes

Additional Info

CLAassistant commented Feb 1, 2024 • edited Loading

michaelsproul commented Feb 1, 2024

mask-pp commented Feb 2, 2024

realbigsean commented Apr 4, 2024

realbigsean commented Apr 4, 2024

chong-he left a comment

Choose a reason for hiding this comment

chong-he Apr 23, 2024

Choose a reason for hiding this comment

mask-pp Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

chong-he Apr 23, 2024

Choose a reason for hiding this comment

chong-he Apr 23, 2024

Choose a reason for hiding this comment

mask-pp Apr 23, 2024

Choose a reason for hiding this comment

mask-pp commented Apr 23, 2024

mask-pp commented Apr 23, 2024

chong-he commented Apr 24, 2024 • edited Loading

mask-pp commented Apr 24, 2024 • edited Loading

chong-he commented Apr 24, 2024

mask-pp commented Apr 25, 2024 • edited Loading

ackintosh left a comment

Choose a reason for hiding this comment

mask-pp commented Apr 26, 2024

chong-he commented Apr 29, 2024

ackintosh commented Apr 30, 2024

ackintosh left a comment

Choose a reason for hiding this comment

mask-pp commented May 4, 2024

dapplion commented Jun 27, 2024

feat(scripts): fix bug about `sed -i` #5169

feat(scripts): fix bug about `sed -i` #5169

mask-pp commented Feb 1, 2024 •

edited

Loading

CLAassistant commented Feb 1, 2024 •

edited

Loading

mask-pp Apr 23, 2024 •

edited

Loading

chong-he commented Apr 24, 2024 •

edited

Loading

mask-pp commented Apr 24, 2024 •

edited

Loading

mask-pp commented Apr 25, 2024 •

edited

Loading