Fix make dev-install
#2372
Fix make dev-install
#2372
Conversation
src/webui/yarn.lock
Outdated
| @@ -5593,7 +5593,7 @@ load-json-file@^4.0.0: | |||
| pify "^3.0.0" | |||
| strip-bom "^3.0.0" | |||
|
|
|||
| loader-fs-cache@>=1.0.3, loader-fs-cache@^1.0.0: | |||
| loader-fs-cache@^1.0.0: | |||
There was a problem hiding this comment.
This line was automatically modified on make.
| @@ -205,14 +205,14 @@ install-node-modules: | |||
| .PHONY: dev-install-node-modules | |||
| dev-install-node-modules: | |||
| #$(_INFO) Installing NNI Package $(_END) | |||
| rm -rf $(NNI_PKG_FOLDER) | |||
There was a problem hiding this comment.
It may need to test what if make install and then make dev-install.
There was a problem hiding this comment.
You should never do that. Remove NNI first before switching to another installation setup.
We don't want to investigate what will happen if pip is requested to overwrite a normal package with an egg package.
There was a problem hiding this comment.
Suggest to keep this line. Don't rely on human to do right things, and the normal install also have this line.
There was a problem hiding this comment.
I don't agree. Removing this folder does NOT uninstall NNI from user's system, instead it leads to an ambiguous "partially installed and partially removed" state.
If a developer do something wrong, I prefer to fail early.
There was a problem hiding this comment.
Any ambiguous state should be fixed in the Makefile. This change makes dev/normal installs script inconsistent, and dig an unnecessary pit. Anyway, if you still prefer, go ahead merge it.
There was a problem hiding this comment.
If you make install twice, it will be okay both with or without this folder, since pip and yarn are allowed to run multiple times with same options. Maybe this folder is removed in case the last bulid was corrupted.
But if someone make install then make dev-install, I don't know what will happen. Because the options feeded to pip are changed, and it's somewhat difficult to figure out pip's exact behaviour in such scenario.
If we want to ensure a clean install, we should trigger clean and uninstall targets instead of removing one or two folders. But as far as I know this is not the common practice of Makefile.
We can take a minute to discuss this issue on next scrum before merging it.
|
@liuzhe-lz please solve conflict |
No description provided.