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

Support getting spec Git info when active shell differs in syntax from bash #22454

Closed
astone123 opened this issue Jun 22, 2022 · 6 comments · Fixed by #22741
Closed

Support getting spec Git info when active shell differs in syntax from bash #22454

astone123 opened this issue Jun 22, 2022 · 6 comments · Fixed by #22741
Assignees

Comments

@astone123
Copy link
Contributor

What would you like?

I'd like to be able to see Git info for my project even if my active shell uses a different syntax than bash. Currently I use Fish Shell as my default shell on Mac. When Cypress tries to load Git info for my project, it uses syntax that isn't supported by my active shell (Fish in this case).

The result is that my project's Git info doesn't show in the spec list

[cy:open:dev:95630]:   cypress:data-context:sources:GitDataSource command execution error: { stdout: '', stderr: "fish: Unsupported use of '='. In fish, please use 'set IFS $'\n''.", code: 0, failed: false, killed: false, signal: null, cmd: "IFS=$'\n" + `'; for file in {"/Users/adams/projects/cypress-example-kitchensink/cypress/e2e/2-advanced-examples/actions.cy.js", ... 

Screen Shot 2022-06-22 at 10 31 44 AM

Why is this needed?

This improvement would be useful for people who use different default shells and run Cypress on their machine.

Other

When we execute the command to get the Git info, we use the active shell and fall back to bash.

const result = await execa(cmd, { shell: process.env.SHELL || '/bin/bash', cwd: this.#gitBaseDir })

Forcing the use of bash fixes the issue in my case but I imagine that there are some cases where we need to use the active shell.

@astone123
Copy link
Contributor Author

Some historical context here - we ended up using a shell command to get Git info here because we couldn't find a good light-weight "Git client" that we could use to get the info.

If we're looking to do a re-write of this that uses a library with bindings for working with Git (which should avoid this shell issue), https://github.com/desktop/dugite looks promising.

@mike-plummer
Copy link
Contributor

Just to provide more context, it looks like this was added to work around issues with the default shell (/bin/sh) used by execa. The current shell selection logic was added here - I imagine @lmiller1990 ran into a situation where some Linux flavors don't include Bash necessitating the use of process.env.SHELL.

We could look into a custom git NPM lib to remove our dependency on manually invoking commands here, but I believe that was investigated earlier and performance issues were encountered. The (perhaps simpler) alternative might be to see if there's a way to restructure the command to use variable substitution syntax compatible with /bin/sh so we can just use the default shell.

There's also some context around the custom exit code handling in this JIRA ticket, just for full context: https://cypress-io.atlassian.net/browse/UNIFY-1794

@lmiller1990
Copy link
Contributor

If anyone can find a fast way to do this with a Node git client, that'd be great, but I don't know if you can or not. You can't do a "give me all the last modified date/status" with raw git, and thus no wrappers can do it, either. This is the fastest way I could find.

I can't remember the exact problem but @mike-plummer is correct, there is some edge case /bin/sh doesn't work on, it must be /bin/bash.

We could do an audit of popular linux OSes, but forcing /bin/bash works great assuming they all ship it. The default on mac is now zsh, and this command works great with that too, so we could also have a list of "known supported shells" we try, perhaps?

Alternative would be including a fish compat syntax, but I don't know how to do that, and I don't know the added complexity is better than just forcing /bin/bash or even reworking this to work with /bin/sh.

@mike-plummer
Copy link
Contributor

Thanks for the context, @lmiller1990. I poked at this a little bit before the break and it seems like we should be able to use /bin/sh with a minor tweak to the variable syntax - I've attached a patch with the changes I made which appear to work locally. I didn't really run it through all the edge cases though (unusual filenames, etc) so maybe there's a gotcha somewhere

base-shell-git-cmd-patch.txt

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jul 15, 2022

The code for this is done in cypress-io/cypress#22741, but has yet to be released.
We'll update this issue and reference the changelog when it's released.

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jul 19, 2022

Released in 10.3.1.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v10.3.1, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Jul 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants