Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: config-helper write mishap with conditional replacements #241

Closed
molleweide opened this issue Aug 11, 2024 · 15 comments
Closed

Bug: config-helper write mishap with conditional replacements #241

molleweide opened this issue Aug 11, 2024 · 15 comments
Labels
bug Broken or unexpected

Comments

@molleweide
Copy link
Collaborator

molleweide commented Aug 11, 2024

In the example below, which is from my setup.bash file, upon auto-rewriting the file, dorothy removed the second instance of GO_INSTALL when it attempted to move strong box to setup-util which resulted in only the closing parenthesis being left.

BEFORE:

if "$DOROTHY/commands/is-mac"; then
	GO_INSTALL=(
		changkun.de/x/rmtrash
		github.com/gennaro-tedesco/boilit # manual install -> `go get -u -v github.com/gennaro-tedesco/boilit`
		github.com/uw-labs/strongbox
		github.com/x-motemen/ghq@latest
		# github.com/cloudflare/utahfs/cmd/utahfs-client
		# https://github.com/maaslalani/nap # cli snippets manager
	)
else
	GO_INSTALL=(
		changkun.de/x/rmtrash
		github.com/gennaro-tedesco/boilit
		github.com/uw-labs/strongbox
		github.com/x-motemen/ghq@latest
		# github.com/rfjakob/gocryptfs: use `setup-util-gocryptfs` instead, as this version lags behind
	)
fi

AFTER:

if "$DOROTHY/commands/is-mac"; then
  GO_INSTALL=(
  	'changkun.de/x/rmtrash'
  	'github.com/gennaro-tedesco/boilit'
  	'github.com/x-motemen/ghq@latest'
  )
else
   ### SYNTAX ERROR HERE... ###
    changkun.de/x/rmtrash
    github.com/gennaro-tedesco/boilit
    github.com/uw-labs/strongbox
    github.com/x-motemen/ghq@latest
    # github.com/rfjakob/gocryptfs: use `setup-util-gocryptfs` instead, as this version lags behind
  )
fi
@balupton
Copy link
Member

balupton commented Aug 12, 2024

Can you provide the entire before, after, and expected

If you want to have a go at writing a test for it and fixing this, it would be config-helper. Seems like the regex needs to be changed to shortest match instead of longest match, which should be done via a question mark character

@molleweide
Copy link
Collaborator Author

molleweide commented Aug 12, 2024

Yeah perfect, i'll look into and study the config helper file. i suspected that was the root cause

@balupton
Copy link
Member

Any news?

@molleweide
Copy link
Collaborator Author

molleweide commented Aug 19, 2024

I spent a lot of time this week just studying related dorothy commands. Reading up on shit.

If one has a large multi conditional in setup.bash, eg.

if mac
...
elif wsl
...
elif ubuntu
...
elif linux
...

How do we want this to be handled I am thinking about?? I havent concluded anything because I need to play around more.

There can potentially be a lot of similar looking PKG_INSTALL sections.


@balupton
Copy link
Member

https://github.com/bevry/dorothy/blob/master/docs/scripting/conventions.md#use-if-not-magic

@molleweide
Copy link
Collaborator Author

molleweide commented Aug 19, 2024

Hmm, i just realized that i dont even know exactly why i had the is-mac statement in the first place - IIRC it had something to do with gocrypfs but now since it is moved into setup-util I dont have the problem anymore.

I have been sketching on some solutions for conditionals but since I dont know why i had the conditional to begin with, I am going to remove it for my configs, and then fixing this bug doesn't make any sense.

closing..

ofc there might still be some things that need to be modified with config-helper but we'll cross that bridge when..

However, I have learnt a lot from digging into this.

@balupton
Copy link
Member

I think the bug still needs to be fixed. I think the best option will be to rewrite config-helper from a ripgrep, sd, teip combo into an AWK script instead.

@molleweide
Copy link
Collaborator Author

molleweide commented Aug 20, 2024

If we do a big rewrite we should/have to consider this
https://ast-grep.github.io/
https://ast-grep.github.io/guide/quick-start.html
https://ast-grep.github.io/catalog/typescript/#typescript <<< since I know you are an expert in TS.

It allows for making syntax based queries on the code instead of using regex type solutions.

Bash is supported as builtin
https://ast-grep.github.io/reference/languages.html

playground:
https://ast-grep.github.io/playground.html

treesitter:
https://tree-sitter.github.io/tree-sitter/
https://github.com/tree-sitter/tree-sitter/wiki/List-of-parsers << quite many parsers exist now

VSCode extension: https://marketplace.visualstudio.com/items?itemName=ast-grep.ast-grep-vscode#overview


https://www.youtube.com/results?search_query=ast-grep
interview with creator: https://www.youtube.com/watch?v=Xs_NDy1U2Fk

@balupton balupton changed the title [BUG]: setup.bash -> does not account for all instances of eg. GO_INSTALL Bug: config-helper write mishap with conditional replacements Aug 27, 2024
@balupton balupton reopened this Aug 27, 2024
@balupton
Copy link
Member

ast-grep seems perfect, however it would then become an essential dorothy dependency and thus constrain which platforms dorothy is available on, furthermore the regexps are already done and a gawk solution, which we already have to make an essential dependency — #239 — will still eliminate the need for 3 rust tools (sd, ripgrep, teip).

I will look into ast-grep's platform support, if it does support all our targets (WSL2, amd64, arm64, risc5, apple intel, apple silicon) then will be fine to become an essential dependency, in addition to the gawk essential dep requirement.

@balupton
Copy link
Member

It doesn't support RISC5 at this point:

@balupton balupton added the bug Broken or unexpected label Aug 27, 2024
@balupton
Copy link
Member

ast-grep will actually be fine for riscv, as it provides itself via cargo compilation, which is already what we do for all our other rust essential deps.

gawk will not be suitable as a replacement, as we use very advanced regexps which rust regex is suitable for and is what we already use, which gawk does not. However, there is frawk:
https://github.com/ezrosent/frawk

So will go with ast-grep or frawk.

@balupton
Copy link
Member

Frawk doesn't provide any pre-built binaries:
ezrosent/frawk#53

@balupton
Copy link
Member

balupton commented Aug 29, 2024

For the OP bug:

before:

GO_INSTALL=()
if is-mac; then
	GO_LINTING_INSTALL='yes'
	GO_INSTALL+=(
		'changkun.de/x/rmtrash'
		# 'github.com/cloudflare/utahfs/cmd/utahfs-client'
	)
fi

if "$DOROTHY/commands/is-mac"; then
	GO_INSTALL=(
		changkun.de/x/rmtrash
		github.com/gennaro-tedesco/boilit # manual install -> `go get -u -v github.com/gennaro-tedesco/boilit`
		github.com/uw-labs/strongbox
		github.com/x-motemen/ghq@latest
		# github.com/cloudflare/utahfs/cmd/utahfs-client
		# https://github.com/maaslalani/nap # cli snippets manager
	)
else
	GO_INSTALL=(
		changkun.de/x/rmtrash
		github.com/gennaro-tedesco/boilit
		github.com/uw-labs/strongbox
		github.com/x-motemen/ghq@latest
		# github.com/rfjakob/gocryptfs: use `setup-util-gocryptfs` instead, as this version lags behind
	)
fi

after setup-go:

GO_INSTALL=(
	'changkun.de/x/rmtrash'
	'github.com/gennaro-tedesco/boilit'
	'github.com/x-motemen/ghq@latest'
)
if is-mac; then
	GO_LINTING_INSTALL='yes'
	GO_INSTALL+=(
		'changkun.de/x/rmtrash'
		# 'github.com/cloudflare/utahfs/cmd/utahfs-client'
	)
fi

if "$DOROTHY/commands/is-mac"; then
		changkun.de/x/rmtrash
		github.com/gennaro-tedesco/boilit # manual install -> `go get -u -v github.com/gennaro-tedesco/boilit`
		github.com/uw-labs/strongbox
		github.com/x-motemen/ghq@latest
		# github.com/cloudflare/utahfs/cmd/utahfs-client
		# https://github.com/maaslalani/nap # cli snippets manager
	)
else
		changkun.de/x/rmtrash
		github.com/gennaro-tedesco/boilit
		github.com/uw-labs/strongbox
		github.com/x-motemen/ghq@latest
		# github.com/rfjakob/gocryptfs: use `setup-util-gocryptfs` instead, as this version lags behind
	)
fi

Here is the intended behaviour:

before:

GO_INSTALL=(
	# github.com/cloudflare/utahfs/cmd/utahfs-client
	# github.com/rfjakob/gocryptfs: use `setup-util-gocryptfs` instead, as this version lags behind
	# https://github.com/maaslalani/nap # cli snippets manager
	changkun.de/x/rmtrash
	github.com/gennaro-tedesco/boilit # manual install -> `go get -u -v github.com/gennaro-tedesco/boilit`
	github.com/uw-labs/strongbox
	github.com/x-motemen/ghq@latest
)

after:

GO_INSTALL=(
	'changkun.de/x/rmtrash'
	'github.com/gennaro-tedesco/boilit'
	'github.com/x-motemen/ghq@latest'
)

Here is the call:

config-helper --file=/Users/balupton/.local/share/dorothy/user/config/setup.bash -- --field=GO_INSTALL '--array=changkun.de/x/rmtrash
github.com/gennaro-tedesco/boilit
github.com/x-motemen/ghq@latest' --field=SETUP_UTILS '--array=1password-cli
aria2
bash
bat
bottom
curl
delta
deno
fish
git
gocryptfs
jq
nu
obs
prettier
ripgrep
sd
strongbox
tealdeer
teip
tree
vim'

@balupton
Copy link
Member

I will fix this by just fixing the regular expressions in config-helper. @molleweide if you want to experiment with ast-grep I'll leave that for you.

@balupton
Copy link
Member

Fixed in dev, but not ideal. Will need to document and communicate the complexity to the user.

balupton added a commit that referenced this issue Sep 17, 2024
/close #249 #241 #239

changes:

- replace common gsed/sed/sd/ripgrep/rg/grep/teip usage with new `echo-regexp` that uses deno, affects `convert-helper`, `config-helper`, `echo-magnet-hash`, `fs-(bytes|kilobytes|megabytes)`, `fs-speed`, `get-devices`, `get-local-to-remote`, `get-opensuse-release`, `get-volumes`, `git-helper`, `is-bash-version-outdated`, `mount-helper`, `network-interface`, `setup-dns`, `setup-util`, `setup-util-1password-cli`, `setup-util-nerd-fonts`, `setup-util-node`, `unziptar`, `what-is-my-(dns|gateway|interface|ip|submit)`, `get-codec`, `is-prefix`, `is-suffix`, `itunes-owners`, `mac-address-helper`, `setup-server`
- `bash.bash`:
    - simplify eval statements and support spaces in paths
    - add explicit names for `__print*` helpers, and slightly change their functionality to be aligned with expectations
        - **please change `__print_line ...` to `__print_lines ...` as in the future `__print_line` may not support arguments**
    - **removed deprecated non `__` prefixed aliases, e.g. `require_globstar` is now only `__require_globstar`**
- `ci/dorothy-workflow`: don't test PRs until review requested
- `ci/dorothy`: don't eslint as no Node.js code
- `ci/vscode`: add an awk syntax highlighting extension, and enable Deno
- `command-working`: give details of failures, add `dash` to exceptions
- `dorothy-config`/`config-helper`: significant work to better handle multiple instances (result is no longer garbled), output and process is now clearer, includes multiple strategies for multiple instances, uses new `echo-regexp` to replace `sd/rg/teip/grep` combo
- `dorothy` minimum deps now only `jq` and `deno`s,
- `down`: add `--url=...` support
- `echo-gnu-command`: fix it not outputting the command after installation
- `echo-if-empty`/`get-devices`/`get-local-to-remote`: fix sporadic success by adding missing `--stdin`
- `echo-lines`: now support `--[no-]quote` flags as alias for prior `--[no-]quoted` flag
- `echo-math`/`echo-(html|url)-(encode|decode)`: rewrote and added tests, now they work correctly, in the latter case `echo-(html|url)-coder` is now used
- `echo-wrap`: now awk compatible, removing dependency on `gawk`
- `eval-tester`: output escaped comparison if special characters were used
- `fs-(bytes|kilobytes|megabytes)`: use `dust` instead of `du` to achieve macos support, also fix crash in `fs-megabytes`
- `fs-rm`: fix trash error on alpine, as trash isn't available on alpine
- `get-codec`: removed `--trim` and made it default (prior verbosity available via `--verbose`), now sensible error if `ffprobe` is not available
- `get-codec`: rewrote and fixed
- `get-filesystem`: no longer fail silently when no args are provided
- `get-local-to-remote`: improve styling
- `get-opensuse-release`: simpler technique
- `get-url-(domain|protocol)` split into `get-url-(domain|protocol).ts` and add tests
- `is-match`: deprecated and moved in beta commands, as it is replaced by `echo-regexp -q` which it now uses internally
- `mac-address-helper`: fixed broken `new` functionality, and improved formatting of outputs
- `set-hostname`/`setup-hosts`/`setup-shell`: fix break when gsed isn't available, now uses the `echo-gnu-command` helper
- `setup-util-1password`: install stable instead of beta, as stable is now stable
- `setup-util-deno`: add apk, uninstall snap (as snap does not allow script execution), use official arm builds as that is now a thing, don't use official yet stupid `deno_install` script
- `setup-util-devel`: add `automake` for macos
- `setup-util-gawk`: add more package systems
- `setup-util-gsed`: add bottles for macos, and add uninstall build support
- `setup-util`: rename `DOWNLOAD_BUILD_EVAL` to `DOWNLOAD_BUILD_INSTALL` and add `DOWNLOAD_BUILD_UNINSTALL`, affected `setup-util-bash`, `setup-util-gawk`, `setup-util-gsed`
- `sparse-vault`: fix broken awk script, use `echo-regexp` instead
- `sparse-vault`: fix broken macos version fetching
- `styles.bash` make code styles more readable on light theme by swapping from dim to intense black, add code error, info, and notice styles
- `unziptar`: fix alpine compatibility
- `what-is-my-(dns|gateway|inteface|ip|subnet)`: rewrote and fixed
- add `.gitattributes` to prevent formatting issues in tests
- add `echo-escape-backslashes` and remove its functionality from `echo-escape-regexp-replacement` for compat with JavaScript Regular Expression
- add `fs-diff` to compute differences between files
- add `get-file` and `setup-util-file` to fix `unziptar` compat on systems without `file` preinstalled
- add `setup-util-frawk`
- add tests to `echo-math`, `echo-(html|url)-(coder|encode|decode)`, `echo-magnet-hash`, `fs-(bytes|kilobytes|megabytes)`, `get-url-(domain|protocol)`

needs more testing:

- `get-opensuse-release`
- `mount-helper`
- `setup-dns`
- `setup-server`
- `setup-util-gsed` on fresh macos
- `unziptar` on macos dmg files

squashes:

- be4b3ce
- ada62e0
- 6860035
- d86e060
- 2b24208
- 1fdae98
- 4506b7f
- 6618689
- 7022dba
- fe5050b
- 0e02dc9
- 7c7265d
- bb507bd
- 818d5c0
- b91656b
- 96d91bc
- 6838a37
- 7c7ed59
- a1f96ea
- 2e4d9da
- e2294c4
- 2a83b20
- da3cf12
- d334b8f
- 86de699
- 35593fe
- 62d42ca
- c723ea2
- e68e2e0
- 7deab4e
- ecee164
- 7cc4031
- e035a22
- e9a955a
balupton added a commit that referenced this issue Sep 17, 2024
/close #249 #241 #239

changes:

- replace common gsed/sed/sd/ripgrep/rg/grep/teip usage with new `echo-regexp` that uses deno, affects `convert-helper`, `config-helper`, `echo-magnet-hash`, `fs-(bytes|kilobytes|megabytes)`, `fs-speed`, `get-devices`, `get-local-to-remote`, `get-opensuse-release`, `get-volumes`, `git-helper`, `is-bash-version-outdated`, `mount-helper`, `network-interface`, `setup-dns`, `setup-util`, `setup-util-1password-cli`, `setup-util-nerd-fonts`, `setup-util-node`, `unziptar`, `what-is-my-(dns|gateway|interface|ip|submit)`, `get-codec`, `is-prefix`, `is-suffix`, `itunes-owners`, `mac-address-helper`, `setup-server`
- `bash.bash`:
    - simplify eval statements and support spaces in paths
    - add explicit names for `__print*` helpers, and slightly change their functionality to be aligned with expectations
        - **please change `__print_line ...` to `__print_lines ...` as in the future `__print_line` may not support arguments**
    - **removed deprecated non `__` prefixed aliases, e.g. `require_globstar` is now only `__require_globstar`**
- `ci/dorothy-workflow`: don't test PRs until review requested
- `ci/dorothy`: don't eslint as no Node.js code
- `ci/vscode`: add an awk syntax highlighting extension, and enable Deno
- `command-working`: give details of failures, add `dash` to exceptions
- `dorothy-config`/`config-helper`: significant work to better handle multiple instances (result is no longer garbled), output and process is now clearer, includes multiple strategies for multiple instances, uses new `echo-regexp` to replace `sd/rg/teip/grep` combo
- `dorothy` minimum deps now only `jq` and `deno`s,
- `down`: add `--url=...` support
- `echo-gnu-command`: fix it not outputting the command after installation
- `echo-if-empty`/`get-devices`/`get-local-to-remote`: fix sporadic success by adding missing `--stdin`
- `echo-lines`: now support `--[no-]quote` flags as alias for prior `--[no-]quoted` flag
- `echo-math`/`echo-(html|url)-(encode|decode)`: rewrote and added tests, now they work correctly, in the latter case `echo-(html|url)-coder` is now used
- `echo-wrap`: now awk compatible, removing dependency on `gawk`
- `eval-tester`: output escaped comparison if special characters were used
- `fs-(bytes|kilobytes|megabytes)`: use `dust` instead of `du` to achieve macos support, also fix crash in `fs-megabytes`
- `fs-rm`: fix trash error on alpine, as trash isn't available on alpine
- `get-codec`: removed `--trim` and made it default (prior verbosity available via `--verbose`), now sensible error if `ffprobe` is not available
- `get-codec`: rewrote and fixed
- `get-filesystem`: no longer fail silently when no args are provided
- `get-local-to-remote`: improve styling
- `get-opensuse-release`: simpler technique
- `get-url-(domain|protocol)` split into `get-url-(domain|protocol).ts` and add tests
- `is-match`: deprecated and moved in beta commands, as it is replaced by `echo-regexp -q` which it now uses internally
- `mac-address-helper`: fixed broken `new` functionality, and improved formatting of outputs
- `set-hostname`/`setup-hosts`/`setup-shell`: fix break when gsed isn't available, now uses the `echo-gnu-command` helper
- `setup-util-1password`: install stable instead of beta, as stable is now stable
- `setup-util-deno`: add apk, uninstall snap (as snap does not allow script execution), use official arm builds as that is now a thing, don't use official yet stupid `deno_install` script
- `setup-util-devel`: add `automake` for macos
- `setup-util-gawk`: add more package systems
- `setup-util-gsed`: add bottles for macos, and add uninstall build support
- `setup-util`: rename `DOWNLOAD_BUILD_EVAL` to `DOWNLOAD_BUILD_INSTALL` and add `DOWNLOAD_BUILD_UNINSTALL`, affected `setup-util-bash`, `setup-util-gawk`, `setup-util-gsed`
- `sparse-vault`: fix broken awk script, use `echo-regexp` instead
- `sparse-vault`: fix broken macos version fetching
- `styles.bash` make code styles more readable on light theme by swapping from dim to intense black, add code error, info, and notice styles
- `unziptar`: fix alpine compatibility
- `what-is-my-(dns|gateway|inteface|ip|subnet)`: rewrote and fixed
- add `.gitattributes` to prevent formatting issues in tests
- add `echo-escape-backslashes` and remove its functionality from `echo-escape-regexp-replacement` for compat with JavaScript Regular Expression
- add `fs-diff` to compute differences between files
- add `get-file` and `setup-util-file` to fix `unziptar` compat on systems without `file` preinstalled
- add `setup-util-frawk`
- add tests to `echo-math`, `echo-(html|url)-(coder|encode|decode)`, `echo-magnet-hash`, `fs-(bytes|kilobytes|megabytes)`, `get-url-(domain|protocol)`

needs more testing:

- `get-opensuse-release`
- `mount-helper`
- `setup-dns`
- `setup-server`
- `setup-util-gsed` on fresh macos
- `unziptar` on macos dmg files

squashes:

- be4b3ce
- ada62e0
- 6860035
- d86e060
- 2b24208
- 1fdae98
- 4506b7f
- 6618689
- 7022dba
- fe5050b
- 0e02dc9
- 7c7265d
- bb507bd
- 818d5c0
- b91656b
- 96d91bc
- 6838a37
- 7c7ed59
- a1f96ea
- 2e4d9da
- e2294c4
- 2a83b20
- da3cf12
- d334b8f
- 86de699
- 35593fe
- 62d42ca
- c723ea2
- e68e2e0
- 7deab4e
- ecee164
- 7cc4031
- e035a22
- e9a955a
@balupton balupton added this to the Share Launch [v2] milestone Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Broken or unexpected
Development

No branches or pull requests

2 participants