Skip to content

git clean deletes ts client #6845

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.

Sign up for GitHub

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

Jump to bottom
Closed
Tratcher opened this issue Jan 18, 2019 · 36 comments
Closed

git clean deletes ts client #6845

Tratcher opened this issue Jan 18, 2019 · 36 comments
Assignees
@Tratcher
Labels
area-signalr Includes: SignalR clients and servers task
Milestone
5.0.0-preview1

Comments

@Tratcher
Copy link
Member

git clean -xdf keeps deleting src\SignalR\clients\ts\signalr and src\SignalR\clients\ts\signalr-protocol-msgpack

Repro steps:

  • build /t:restore (runs npm install)
  • dir /al /s (shows those folders as hard junctions from \src\SignalR\clients\ts\FunctionalTests\node_modules\@aspnet)
  • git clean -xdf (those folders get deleted)
  • git status (git shows those folders as deleted)

This happens because we've configured a junction between src\SignalR\clients\ts\FunctionalTests\node_modules\@aspnet\signalr and src\SignalR\clients\ts\signalr (how?) and node_modules/ is in the .gitignore file. When git clean clears the node_modules dir it follows the junction and deletes the client as well.

Proposal: can we populate the node_modules\@aspnet\signalr folder via some other mechanism besides using a junction? E.g. manually copy the files?

Open question: Where is this junction configured? Is it this?
https://github.com/aspnet/AspNetCore/blob/89a7f3cf774d46eaaea26861744a224a6487f31c/src/SignalR/clients/ts/tsconfig.base.json#L21-L28

@davidfowl @BrennanConroy @anurse
@Tratcher Tratcher added task area-signalr Includes: SignalR clients and servers labels Jan 18, 2019
@BrennanConroy
Copy link
Member

BrennanConroy commented Jan 18, 2019 via email

@natemcmaster
Copy link
Contributor

https://github.com/aspnet/AspNetCore/blob/c2a0f894482895aa5af6e80dafa1e22724d5bd8f/src/SignalR/clients/ts/FunctionalTests/package.json#L7-L9

@natemcmaster
Copy link
Contributor

Looks like a known issue in git. See git-for-windows/git#437 and https://www.bountysource.com/issues/29901518-git-clean-removes-content-of-symlinks

@muratg
Copy link
Contributor

muratg commented Jan 18, 2019

@BrennanConroy I think we should look into removing symbolic link usage :(

@analogrelay
Copy link
Contributor

As @natemcmaster cited, the path references in package.json cause npm to create the link. This is a very common pattern for path-based references between node modules. If it causes problems with git for windows that's concerning. I'm not sure if there's a simple way to work around it.

@muratg
Copy link
Contributor

muratg commented Jan 24, 2019

@anurse I was thinking that we could do dependencies differently. Perhaps not possible, but I thought there could be a way. Multiple people keep hitting this on Windows unfortunately. It's easy to revert but it's definitely a nuisance.

@natemcmaster
Copy link
Contributor

Does yarn have a solution for this?

@bradygaster
Copy link
Member

@natemcmaster do you think a mitigation possibility could be that we move the TS client out of the mondo repo and begin the process of separating the non-.NET SignalR clients into their own repositories somewhere else? If that would mitigate here, we could start talking about doing it soon.

@natemcmaster
Copy link
Contributor

natemcmaster commented Jan 24, 2019 via email

@Tratcher
Copy link
Member Author

@natemcmaster but it does reduce the number of people affected by it, and if you're planning to separate the clients anyways...

@BrennanConroy
Copy link
Member

Moving to a new repo solves the problem of everyone complaining. Only a couple people will hit the issue if we move it, and we can ignore it.

@muratg
Copy link
Contributor

muratg commented Jan 24, 2019

IMO, let's not jump to a "new repo" solution just yet. I'd rather have simplified build infra with less repos. Even if it means we can't solve this fully.

@analogrelay
Copy link
Contributor

Does yarn have a solution for this?

Maybe. Worth a shot. The biggest place this is used is in the Functional Tests and they use WebPack. We should probably use a standard version reference instead of a path reference and then use the WebPack config to redirect to the local path.

@muratg muratg added this to the Backlog milestone Feb 21, 2019
@ryanbrandenburg
Copy link
Contributor

Any chance our new usages of yarn make this simpler? It's always very annoying when I run into this.

@natemcmaster
Copy link
Contributor

natemcmaster commented Mar 29, 2019 via email

@analogrelay
Copy link
Contributor

analogrelay commented May 17, 2019 •
edited
Loading

I just had a crazy brilliant idea. This occurs because git clean cleans the node_modules folder in SignalR, including things like node_modules/@aspnet/signalr, which is just a junction to the actual source directory. Git cleans through the link and that wipes everything out.

Given that, the solution is actually super simple. Add node_modules/@aspnet/signalr (and node_modules/@aspnet/signalr-protocol-msgpack to the .gitignore as unignored ;):

!node_modules/@aspnet/signalr
!node_modules/@aspnet/signalr-protocol-msgpack

We could do this just in the src/SignalR directory. That would mean that the node_modules directory in those folders hangs around after a clean but it only keeps the symlinks, which is fine.

@BrennanConroy could you see if this fixes the issue? If so, let's do it. (cc @Tratcher @aspnet/build)

@analogrelay analogrelay modified the milestones: Backlog, 3.0.0-preview6 May 17, 2019
@BrennanConroy
Copy link
Member

We would need to commit the symlink folders otherwise they would probably appear in git status

@dougbu
Copy link
Member

dougbu commented May 17, 2019

either way, I likes it 😃

@analogrelay
Copy link
Contributor

analogrelay commented May 17, 2019 •
edited
Loading

@BrennanConroy true, but the files themselves are already committed anyway so it might just work? There could be a recursion issue though since I think the node_modules folder is still under there (ad infinitum)... Beware when trying this out @BrennanConroy, you might hang git ;).

@BrennanConroy
Copy link
Member

Tried various incantations of the above but couldn't get it working

@analogrelay
Copy link
Contributor

Back to the drawing board I guess :(.

@analogrelay
Copy link
Contributor

There is an issue tracking this in git for windows and it seems like there's a possibility it will be fixed git-for-windows/git#607

@analogrelay analogrelay removed this from the 3.0.0-preview6 milestone May 21, 2019
@analogrelay analogrelay modified the milestones: 3.0.0-preview8, Backlog Jul 9, 2019
@BrennanConroy BrennanConroy mentioned this issue Jul 15, 2019
Closed
dougbu added a commit that referenced this issue Jul 17, 2019
@dougbu
Change clean.ps1 (and clean.cmd) to restore deleted files after c…
222b862 
…leaning

- #6845
@dougbu dougbu mentioned this issue Jul 17, 2019
Merged
@dougbu dougbu self-assigned this Jul 17, 2019
@dougbu dougbu added the Working label Jul 17, 2019
@dougbu dougbu modified the milestones: Backlog, 3.0.0-preview8 Jul 17, 2019
@dougbu
Copy link
Member

dougbu commented Jul 17, 2019

Found a reasonable way. See #12281

@analogrelay
Copy link
Contributor

I wouldn't call that a fix, but it's a reasonable stop-gap. We shouldn't close this work item based on that, but I will be moving this out from preview 8.

dougbu added a commit that referenced this issue Jul 18, 2019
@dougbu
Change clean.ps1 (and clean.cmd) to restore deleted files after c…
d641dff 
…leaning

- #6845
@dougbu
Copy link
Member

dougbu commented Jul 18, 2019

@anurse what is your preference over what was in d641dff? That may be a workaround but it's reliable.

@analogrelay
Copy link
Contributor

My preference is that git clean not delete the TS files. Getting people to use a non-standard clean command (particularly external contributors) isn't really a suitable long-term solution IMO. I agree it's the best we've got right now though.

@Tratcher
Copy link
Member Author

In other words, keep this open and move it to the backlog.

@dougbu dougbu added 1 - Ready and removed Working labels Jul 18, 2019
@dougbu
Copy link
Member

dougbu commented Jul 18, 2019

Thanks for your perspectives and for unassigning me 😺

@Tratcher
Copy link
Member Author

Tratcher commented Aug 28, 2019 •
edited
Loading

Fixed by git-for-windows/git#2268 in 2.23.0? Needs verification.

@Tratcher Tratcher self-assigned this Aug 28, 2019
@Tratcher
Copy link
Member Author

Fixed !

@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@Tratcher Tratcher

Labels
area-signalr Includes: SignalR clients and servers task
Projects
None yet
Milestone
5.0.0-preview1
Development

No branches or pull requests
9 participants
@analogrelay @muratg @Tratcher @natemcmaster @dougbu @BrennanConroy @ryanbrandenburg @mkArtakMSFT @bradygaster