-
Notifications
You must be signed in to change notification settings - Fork 116
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
packages windows: redirect to GitHub Releases to download latest Groonga #1432
packages windows: redirect to GitHub Releases to download latest Groonga #1432
Conversation
if (target_namespace == :windows) | ||
htaccesss_value = "RewriteRule groonga-((1[2-9]|[2-9][0-9])[\\d\\.]+?)-.+?-vs(?!.*(2012|2013|2015|2017)).+?\\.zip"\ | ||
" #{github_releases_base_url}/v$1/$0\n" | ||
File.write(htaccess_path, htaccesss_value, mode: "w") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Memo:
When vs2019+ reaches EOL, I think we need to add RewriteRules for the latest
version of them at this point.
For example, if vs2019
reaches EOL and #{target_namespace}_targets
not contains vs2019
, we don't create RewriteRules for vs2019
on the lines 191 - 203.
So users will not get latest vs2019
from URLs like https://packages.groonga.org/windows/groonga/groonga-latest-x86-vs2019-with-vcruntime.zip, they will return 404 not found.
In order to avoid 404 not found, we need to add RewriteRules like below.
RewriteRule groonga-latest-x86-vs2019-with-vcruntime.zip https://github.com/groonga/groonga/releases/download/{old-version}/groonga-{old-version}-x86-vs2019-with-vcruntime.zip
If we don't need to provide the way to download EOL versions from packages.groonga.org, the RewriteRules for latest
of EOL versions is needless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
この英語では何を言っているのかわからない可能性があるので日本語でも記載。
191行目から203行目では、現在サポート対象となっているVisual Studioのバージョン(vsXXXX)のlatest
に対するリダイレクトルールを作っている。(groonga-latest-x86-vs2019のようなやつ)
なので、サポート対象外になったvsXXXXのlatest
へのリダイレクトルールは作られない。
毎回新しく.htaccessを作っているので、.htaccessの中に古いバージョン用のlatest
用のリダイレクトルールは作られない。
vs2017まではpackages.groonga.orgに正しいlatest
の実体があるからそれでも問題ないのだが(.htaccessのルールに引っかからなくても、実体のほうが使われるだけだから)、この対応が入った後は、vs2019以降では、packages.groonga.orgに実体がない or 実体が古すぎるという状態になる(この変更の後はpackages.groonga.orgのパッケージは更新しないから)。
そのため、例えばvs2019のサポートが終了して、latest-xxx-vs2019のリダイレクトルールが作られなくなると、 https://packages.groonga.org/windows/groonga/groonga-latest-x86-vs2019-with-vcruntime.zip のようなURLにアクセスした時に、404エラーになるか古すぎるものがダウンロードされるようになる。
なので、例えばvs2019のサポートが終了した際には、このあたりの処理で https://packages.groonga.org/windows/groonga/groonga-latest-x86-vs2019-with-vcruntime.zip からvs2019を最後にサポートしたバージョンへのリダイレクトルールを作ってあげる必要があるのではないかということ。
ただ、サポートが終了したバージョンのlatest
はpackages.groonga.orgからはダウンロードさせなくても良いというのであれば、そのような対処は不要。
なお、latest
を指定せずにバージョンを明示的に指定すれば最初のリダイレクトルールに引っかかるのでリダイレクトされる。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
すでにEOLになったものは、サポート終了しているので、 https://packages.groonga.org/windows/groonga/groonga-latest-x86-vs2019-with-vcruntime.zip でダウンロードできなくてもいいように思います。
EOLになったパッケージはできる限り使ってほしくないので。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
なるほど、であれば特に将来的に追加で対応が必要になることはなさそうですね!
fbc1cc7
to
2c87903
Compare
|
||
if (target_namespace == :windows) | ||
htaccesss_value = "RewriteRule groonga-((1[2-9]|[2-9][0-9])[\\d\\.]+?)-.+?-vs(?!(2012|2013|2015|2017)).+?\\.zip"\ | ||
" #{github_releases_base_url}/v$1/$0\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this rewrite rule? I think that we only need a link to -latest
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had written the reason why this ReqiteRule is needed on description.
The regexp of the first line is for 12.0.0+ (but not latest) and vs2019+.
If we omit this, for example, after Groonga 12.1.0 is released, we won't be able to download old versions by version specifying URL like https://packages.groonga.org/windows/groonga/groonga-12.0.9-x64-vs2019-with-vcruntime.zip because v12.0.9 will not exist on packages.groonga.org.
But if we do not need to provide a way to donwnload old version Groonga from packages.groonga.org, it is needless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I see.
In my opinion, this is needless.
Because a download URL that user can know is only https://packages.groonga.org/windows/groonga/groonga-latest-xxx.zip normally.
In addition, I would like to use in the user the latest version when Groonga up to version.
Because the user doesn't need to use a specific version in when Groonga up to version generally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this RewriteRule
.
@HashidaTKS Could you confirm my comments? |
Fixed? I think that the current behavior (provide packages for Windows on packages.groonga.org) isn't a bug. |
github_releases_base_url = "https://github.com/groonga/groonga/releases/download" | ||
htaccess_path = "#{repositories_dir}/.htaccess" | ||
|
||
if (target_namespace == :windows) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (target_namespace == :windows) | |
if target_namespace == :windows |
@@ -175,19 +175,32 @@ def download_packages(target_namespace) | |||
mkdir_p(repositories_dir) | |||
download_dir = "#{base_dir}/tmp/downloads/#{@version}" | |||
mkdir_p(download_dir) | |||
|
|||
github_releases_base_url = "https://github.com/groonga/groonga/releases/download" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packages-groonga-org-package-task.rb
is shared with other packages that use packages.groonga.org such as Mroonga and PGroonga. So putting groonga/groonga
here isn't a good idea.
end | ||
latest_link_base_name = target.gsub(@version, "latest") | ||
htaccesss_value = "RewriteRule #{latest_link_base_name} #{github_releases_base_url}/v#{@version}/#{target}\n" | ||
File.write(htaccess_path, htaccesss_value, mode: "a+") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid reusing download_packages
for this because the new behavior downloads nothing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have fixed it.
I have created a new task named create_htaccess for createing .htaccess because it seemed that neither
download_packages(target_namespace)nor
release(target_namespace)` is good method for creating .htaccess in it.
How do you feel that?
Changes
- Skip downloading packages when target_namespace is
:windows
. - Create new task named
create_htaccess
for each namespaces.- This task creates
.htaccess
. - It works only when namespaces is
:windows
. Do nothing in other namespaces. - I think there is a more suitable name for this task. Do you have a good idea for the task name?
- This task creates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about prepare
or something? It seems that create_htaccess
is too specific to use in packages-groonga-org-package-task.rb
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to prepare
desc "Make .htaccess for #{target_namespace} packages" | ||
task :create_htaccess do | ||
create_htaccess(target_namespace) if enabled | ||
end | ||
tasks << "#{target_namespace}:create_htaccess" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seem that this isn't related to use_built_package?
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will fixed it.
I focused on the case of redirecting URL so it added to this line because there should be some built packages to redirect.
But as you said, create_htaccess
( will be renamed prepare
) is for not only redirection but also some other cases.
So it should be out of use_built_package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have fixed it.
packages/Rakefile
Outdated
rm_rf(htaccess_path) | ||
|
||
__send__("#{target_namespace}_targets").each do |target| | ||
redirect_url = built_package_url(target_namespace, target) | ||
redirect_target = target.gsub(@version, "latest") | ||
htaccesss_value = "RewriteRule #{redirect_target} #{redirect_url}\n" | ||
File.write(htaccess_path, htaccesss_value, mode: "a+") | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about opening a file instead of calling File.write()
multiple times? I haven't seen the usage (Use FileUtils.rm_f
and multiple File.write(mode: "a+")
to create multi lines content).
rm_rf(htaccess_path) | |
__send__("#{target_namespace}_targets").each do |target| | |
redirect_url = built_package_url(target_namespace, target) | |
redirect_target = target.gsub(@version, "latest") | |
htaccesss_value = "RewriteRule #{redirect_target} #{redirect_url}\n" | |
File.write(htaccess_path, htaccesss_value, mode: "a+") | |
end | |
File.open(htaccess_path, "w") do |htaccess| | |
__send__("#{target_namespace}_targets").each do |target| | |
redirect_url = built_package_url(target_namespace, target) | |
redirect_target = target.gsub(@version, "latest") | |
htaccess.puts("RewriteRule #{redirect_target} #{redirect_url}") | |
end | |
end | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have fixed it.
@@ -244,6 +247,12 @@ def define_release_tasks | |||
end | |||
tasks << "#{target_namespace}:download" | |||
end | |||
|
|||
desc "Prepare for #{target_namespace} packages" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need for
here?
@yoshimotoyuk What do you think about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for
is needless in this case.
メモ:
Prepare: 「準備する」というようなニュアンス
Prepare for: (来たるべき何かに)「備える」というようなニュアンス
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, Prepare for releasing #{target_namespace} packages
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
「アップロード用にパッケージを準備する」タスクだと思っていました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
アップロード用にパッケージを準備する場合は
Prepare #{target_namespace} packages for up-load
になると思います。
"prepare for releasing #{target_namespace} packages"だと {target_namespace}パッケージをリリースするために準備する。となると思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
あざっす。
この文脈だとfor up-load
は冗長なのでPrepare #{target_namespace} packages
でいい気がしました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
話を伺って Prepare #{target_namespace} packages
だけで良いのではないかと思いましたが、いかがでしょうか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
被りました。Prepare #{target_namespace} packages
に変更します。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
はい。前後関係を見るに
シンプルにして大丈夫だと思いました
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
更新しました。
packages/Rakefile
Outdated
__send__("#{target_namespace}_targets").each do |target| | ||
redirect_url = built_package_url(target_namespace, target) | ||
redirect_target = target.gsub(@version, "latest") | ||
htaccess.puts("RewriteRule #{redirect_target} #{redirect_url}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use Redirect
instead of RewriteRule
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed to use RedirectMatch
instead of Redirect
in order to simplify a path specification in source code.
The new .htaccess
is as below.
RedirectMatch ^.+/groonga\-latest\-x64\-vs2022\.zip$ https://github.com/groonga/groonga/releases/download/v12.0.9/groonga-12.0.9-x64-vs2022.zip
RedirectMatch ^.+/groonga\-latest\-x64\-vs2022\-with\-vcruntime\.zip$ https://github.com/groonga/groonga/releases/download/v12.0.9/groonga-12.0.9-x64-vs2022-with-vcruntime.zip
RedirectMatch ^.+/groonga\-latest\-x64\-vs2019\.zip$ https://github.com/groonga/groonga/releases/download/v12.0.9/groonga-12.0.9-x64-vs2019.zip
RedirectMatch ^.+/groonga\-latest\-x64\-vs2019\-with\-vcruntime\.zip$ https://github.com/groonga/groonga/releases/download/v12.0.9/groonga-12.0.9-x64-vs2019-with-vcruntime.zip
RedirectMatch ^.+/groonga\-latest\-x86\-vs2019\.zip$ https://github.com/groonga/groonga/releases/download/v12.0.9/groonga-12.0.9-x86-vs2019.zip
RedirectMatch ^.+/groonga\-latest\-x86\-vs2019\-with\-vcruntime\.zip$ https://github.com/groonga/groonga/releases/download/v12.0.9/groonga-12.0.9-x86-vs2019-with-vcruntime.zip
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to use RedirectMatch
not Redirect
?
It seems that we don't have any pattern (we have only literal).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a first, the rule below was incorrect because it redirects http://packages.groonga.org/windows/packages/groonga-latest-x64-vs2022.zip
to http://packages.groonga.org/windows/packages/https://github.com/groonga/groonga/releases/download/v12.0.8/groonga-12.0.8-x64-vs2022.zip
. We need to specify an absolute path for old URL.
RewriteRule groonga-latest-x64-vs2022.zip https://github.com/groonga/groonga/releases/download/v12.0.8/groonga-12.0.8-x64-vs2022.zip
If we use Redirect
, the rule will be as below.
Redirect /windows/groonga/groonga-latest-x86-vs2019.zip https://github.com/groonga/groonga/releases/download/v12.0.9/groonga-12.0.9-x86-vs2019.zip
The reason why I used RedirectMatch
was just because I wanted to avoid creating /windows/groonga
in source code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about just prepending /windows/groonga/
instead of using regular expression?
/windows/groonga/
will not be changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we can use /#{target_namespace}/#{@package}/
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Thanks.
Or we can use
/#{target_namespace}/#{@package}/
.
I have changed to use it.
Could you update the description to match the latest behavior? |
I have addressed your comments. |
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Thanks! |
Changed to redirect to GitHub Releases by
.htaccess
to download latest Groonga.Changes
prepare
..htaccess
for redirecting when target is Windows.Created
.htaccess
is like below.