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

tools/cmake: fix download url with make variables #10754

Merged
merged 1 commit into from
Sep 19, 2022

Conversation

mpratt14
Copy link
Contributor

Use a make variable pattern for the url
so that only one version number needs to be changed when version is bumped.

Signed-off-by: Michael Pratt mcpratt@pm.me

@github-actions github-actions bot added the build/scripts/tools pull request/issues for build, scripts and tools related changes label Sep 18, 2022
@@ -8,12 +8,13 @@ include $(TOPDIR)/rules.mk

PKG_NAME:=cmake
PKG_VERSION:=3.24.1
PKG_VERSION_MAJOR:=$(word 1,$(subst ., ,$(PKG_VERSION))).$(word 2,$(subst ., ,$(PKG_VERSION)))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wonderful. A lot of packages in the packages feed can take advantage of this.

Copy link
Member

@Ansuel Ansuel Sep 19, 2022

Choose a reason for hiding this comment

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

Wonder... should we make this an helper? @neheb

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not.

@Ansuel
Copy link
Member

Ansuel commented Sep 19, 2022

@mpratt14 I wonder if you can rebase this on top of master so this gets flagged as merged. You disabled maintainers force push so I can't do it and the only option is to manually merge and flag this as closed :(

@Ansuel Ansuel added the ready for merge pull request reviewed and prepared for merge label Sep 19, 2022
@mpratt14
Copy link
Contributor Author

Isn't there a reason that PRs are never merged through github, and instead always manually? something like, github is a one-way mirror and then fast-forward from git.openwrt.org would fail?

that's the reason they added the review block for merging "At least 6 approving reviews are required by reviewers with write access"?? or is it just to prevent accidental misclicks or something

@Ansuel
Copy link
Member

Ansuel commented Sep 19, 2022

@mpratt14 yes but with extra step they can still be flagged as merged as you can see some closed pr recently

The extra step needs to rebase the pr on top of master so they can be merged with -ff-only. Normally I would do this for the user by force pushing it but is not possible if the user block this feature for the pr.

@mpratt14
Copy link
Contributor Author

got it 👍🏼

Use a make variable pattern for the url
so that only one version number needs to be changed
when version is bumped.

Signed-off-by: Michael Pratt <mcpratt@pm.me>
@mpratt14
Copy link
Contributor Author

and by the way, the reason is that I update all my branches together with git push --mirror origin so anyone else pushing my branch would get overwritten and that can cause confusion if I'm working on something else while you are working on this

@openwrt-bot openwrt-bot merged commit 1e726ba into openwrt:master Sep 19, 2022
@Ansuel
Copy link
Member

Ansuel commented Sep 19, 2022

@mpratt14 see merged :D anyway it's ok it was just to clear why force push is needed and why it's problematic. Everyone has it's reason it's just a pitty to close pr without giving actual credits that they were merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build/scripts/tools pull request/issues for build, scripts and tools related changes ready for merge pull request reviewed and prepared for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants