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

fix external editor for cmd and bat on windows #1674

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

pm100
Copy link
Contributor

@pm100 pm100 commented May 4, 2023

fix for #1316

resubmitting after commit cleanup

the actual fix is

  • if execution fails on windows
  • attempt to rerun as cmd /c <cmd> <args> <file name>
  • if that fails then report error

verified with both vim (which is installed a bat file) from the original report and code (which is installed as a cmd file) which was mentioned in a further comment

It is arguable that rust's program exec function should do this itself. But is also arguable that it shouldnt. I will file an issue there

Then I added a bunch of tests. One is generic, the other test this specific fix. I could maybe add more later.

In order to make the tests work I need code from the test plumbing in the asyncgit sub crate. There are 2 choices to do that, given that code flagged as test on one sub crate is not available to other crates in the project.

I chose the second.

Sadly this makes clippy and cargo check see that plumbing code (that is only compiled into tests) as part of the main project. They complain big time about that code. I fixed some complaints, I told clippy to ignore some, but the main (not real issue) is that they see duplicated packages. It is almost impossible to disentangle them. And its not possible to tell clippy and check to skip that fake feature. So i added a skip duplicates check (the same as the main project) and modified deny.toml

I followed the checklist:

  • [ x] I added unittests
  • [ x] I ran make check without errors
  • [ x] I tested the overall application
  • I added an appropriate item to the changelog

@pm100 pm100 changed the title fix external editor for win and bat on windows fix external editor for cmd and bat on windows May 7, 2023
Cargo.lock Outdated
@@ -362,7 +362,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "6d2301688392eb071b0bf1a37be05c469d3cc4dbbd95df672fe28ab021e6a096"
dependencies = [
"quote",
"syn",
"syn 1.0.107",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so far I managed to keep duplicate versions of syn away. can we achieve this here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its only testing - the dup version is needed by test serial. You have the same testing issue when you use test serial but the validation tools dont see it. BUT due to how I pull asyncgit into testing in the main tree it seems to the various tools like this is a dup in the real code, its not. (I commented earlier about using asyncgit code in tests)

I can take out the tests, that solves all.

Or I can duplicate the test scaffolding from asyncgit into the main tree, then the tools wont see it.

Or I can upgrade most things to the same syn (v2) but I would have to pull local versions of three things into gitui tree since they are marked syn=1.x

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there is a way to migrate to syn v2 this would be my preferred solution. this should happen in a separate PR though. can you give this a try? this will set things up for the future

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after cargo update we are left with three things using 1

pm100@pauls-MacBook-Air gitui % cargo tree -i syn@1.0.109 --all-features
syn v1.0.109
├── git-version-macro v0.3.5 (proc-macro)
│   └── git-version v0.3.5
│       └── bugreport v0.5.0
│           └── gitui v0.22.1 (/Users/pm100/oops2/gitui)
├── proc-macro-error v1.0.4
│   └── struct-patch-derive v0.2.3 (proc-macro)
│       └── struct-patch v0.2.3
│           └── gitui v0.22.1 (/Users/pm100/oops2/gitui)
└── struct-patch-derive v0.2.3 (proc-macro) (*)
pm100@pauls-MacBook-Air gitui %

proc-macro-error has 2 PRs open upgrading syn to v2.
git-version-macro has 3 month old PR too
I opened an issue for struct-patch-derive

@pm100
Copy link
Contributor Author

pm100 commented Jun 22, 2023

I have resolved this by getting everybody onto syn v1. Once all the dependencies move to v2 then gitui can move.

I also had to fix a new clippy failure due to a new rule added in 1.71

@extrawurst
Copy link
Owner

depends on #1781

@extrawurst
Copy link
Owner

since #1781 is closed now, can we update this to current master and move it forward?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants