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.

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

Introduce support for upgrading dependencies; Change mbedtls_url to the Github source; fix SC2162; fix SC2181 #118

Closed
wants to merge 8 commits into from
Closed

Introduce support for upgrading dependencies; Change mbedtls_url to the Github source; fix SC2162; fix SC2181 #118

wants to merge 8 commits into from

Conversation

IceCodeNew
Copy link
Contributor

Introduce support for upgrading dependencies

Upgrading dependencies while reinstalling(which is actually means upgrading) shadowsocks.

fix SC2162

Always use -r with read unless you have a good reason not to.
Be aware that read without -r will mangle backslashes!

这个 PR 我的考虑是要么等 PR #117 被通过以后，我变基重新 force-push 一遍这个分支；要么是直接请 @teddysun 关闭 PR #117 ，直接合并这个 PR。
之所以这么做是考虑到我新增的这个功能改动有点大，万一不能一次性过呢？所以把一些无关紧要的修改分到了另一个分支 4pr_icn 上面去，但是为了避免合并 PR 时遇到冲突，所以这个分支是在 4pr_icn 的基础上再添加 commits 的，这样如果最后变成两个 PR 都分开合并的情况，我 git am 还有 git rebase 时方便一些。

IceCodeNew added 5 commits July 2, 2020 03:08
@IceCodeNew
Make Shadowsocks-libev be the preferred option.
fcbbf52
@IceCodeNew
Make wget check certificates.
9c1a984 
我特意在整个 repo 范围内搜寻了一下 `--no-check-certificate` 或者其他可能的关键词，没有看到有人提 issue 说 wget 检查证书导致了错误，所以才加入这个参数来兼容。
那么我的意见是没有必要在一开始就加入这个参数，这降低了安全性，很多源代码都用到了 `wget` 拉下来，有必要通过验证证书确保拉下来的东西是未经篡改过的。
另外以我自己为例，我环境上 `wget` 还是一个 alias: `wget2 --progress=bar --secure-protocol=PFS --https-enforce=soft`，所以我打算一点一点地把脚本中的二进制改为通过绝对路径调用，避免 alias 和 脚本中参数冲突导致问题的可能性。
@IceCodeNew
Several improves
cd5340c 
0. fix SC2068
1. fix SC2196
2. use `[[ … ]]` rather than `[ … ]`
(refer: https://google.github.io/styleguide/shellguide.html#s6.3-tests)
@IceCodeNew
Introduce support for upgrading dependencies
c4f2d67 
Upgrading dependencies while reinstalling(which is actually means upgrading) shadowsocks.
@IceCodeNew
fix SC2162
06a52cb
@IceCodeNew
Copy link
Contributor Author

image
image

这次顺便趁机也测试了以下之前 PR 的使 mbedtls 始终取最新版本的代码，一切工作正常。
但是 mbedtls 在 GitHub 上 release 新的大版本以后半天都没有更新官网，不知道是什么意思。
我建议脚本里下载的地址从官网改成从 GitHub 下载，这个我会以追加 commit 的形式补充在当前 PR 里。

@IceCodeNew
Change mbedtls_url to the Github source;
6d8ef14 
0. Change `mbedtls_url` to the Github source
1. fix SC2181
2. use `[[ … ]]` rather than `[ … ]`
(refer: https://google.github.io/styleguide/shellguide.html#s6.3-tests)
3. Minor fix
@IceCodeNew IceCodeNew changed the title Introduce support for upgrading dependencies; fix SC2162 Introduce support for upgrading dependencies; Change mbedtls_url to the Github source; fix SC2162; fix SC2181 Jul 1, 2020
@IceCodeNew
Copy link
Contributor Author

image
这 GitHub 打包解压出来的目录名真是太弱智了，我无语……（已经添加 commit 修正问题）

@IceCodeNew
Copy link
Contributor Author

已经实机跑了一遍确认没有问题了。我觉得下一个目标就是 CI test 了（

IceCodeNew referenced this pull request Jul 2, 2020
@teddysun
Update comments
1e25c70
@IceCodeNew
Copy link
Contributor Author

@teddysun 可以的话希望能给些意见，这样我好跟着做对应的修改。
个人希望尽快把这项功能支持推进下去。

@teddysun
Copy link
Owner

teddysun commented Jul 3, 2020

#117 已经关闭。
wget --no-check-certificate 是为了最大程度的兼容和避免出错，不建议被替代。
单纯只是 bash 脚本，就不需要用 CI 来集成测试了吧。

@teddysun
Copy link
Owner

teddysun commented Jul 3, 2020

另：type -P wget 的写法也是非必要的。在下载脚本的时候就需要用到 wget，因此不必在脚本里判断了。
双中括号之类的，不必太在意。因为是使用 bash，而非 sh。
因此，打算关闭这个 PR 了

@teddysun teddysun closed this Jul 3, 2020
@IceCodeNew
Copy link
Contributor Author

IceCodeNew commented Jul 3, 2020 •
edited
Loading

#117 已经关闭。
wget --no-check-certificate 是为了最大程度的兼容和避免出错，不建议被替代。
单纯只是 bash 脚本，就不需要用 CI 来集成测试了吧。

CI test 是说笑的，wget 那个修改我可以 drop 掉，但我觉得你 miss 了这个 PR 的主要目的——让脚本安装的 shadowsocks 可以更新，要做到这一点不只是简单重新运行一遍脚本就行了，依赖项也非常有必要升级。

此外对于最近的一个 commit 我也不太理解你的用意，我在那个 commit #1e25c70 下面做了评论，希望能得到你的解答。
@teddysun

@IceCodeNew
Copy link
Contributor Author

另：type -P wget 的写法也是非必要的。在下载脚本的时候就需要用到 wget，因此不必在脚本里判断了。
双中括号之类的，不必太在意。因为是使用 bash，而非 sh。
因此，打算关闭这个 PR 了

type -P wget 不是用来判断 wget 是否存在，而是在一个既存在 wget 二进制文件，又有名为 wget 的 alias 环境下，确保调用的是二进制文件，而不是 alias，因为这样会有可能的参数冲突问题。
至于你说双中括号的改进，正是因为这是 bash，而不是 sh，所以我才希望做这个调整。双中括号不仅性能更高（这一点我看过一些个人做的测试，是否靠谱另说），但是最重要的重点在于双中括号可以避免其中的内容被 shell 释义和展开，同时还支持正则表达式——这也是 Google style 里建议使用双中括号的原因所在。

@IceCodeNew
Copy link
Contributor Author

所以综上，这个 PR 里一些试图规范化部分 shell 代码的部分我们可以再议，但是核心的对依赖项更新的支持功能还没有得到讨论，所以我认为这个 PR 不应该关闭。

@teddysun
Copy link
Owner

teddysun commented Jul 3, 2020

核心的对依赖项更新的支持功能还没有得到讨论

就这点来说，欢迎提交 PR ：）

@IceCodeNew
Copy link
Contributor Author

IceCodeNew commented Jul 3, 2020 •
edited
Loading

核心的对依赖项更新的支持功能还没有得到讨论

就这点来说，欢迎提交 PR ：）

我看要不然您还是把这个 PR 重新打开，我把分支变基一下，只保留与这个功能相关的功能。这样您就可以 merge 了。

对 wget 所作的一切改进已经丢弃掉了。包括由 wget 改为 $(type -P wget) 的 commit（但我还是坚持认为使用后者更好）

@teddysun
Copy link
Owner

teddysun commented Jul 3, 2020

请重新提交 PR 吧，这个已经无法 reopen 了

@IceCodeNew
Copy link
Contributor Author

请重新提交 PR 吧，这个已经无法 reopen 了

请对新的 PR #119 提出修改意见，或合并。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@IceCodeNew @teddysun