Makefile(s) polishing #6
Conversation
- Makefile reordering to be more in sync with OpenWrt packages feed - Sync indentation somewhere were used spaces, somewhere not - Reordered install section to be more readable
There was a problem hiding this comment.
Since your changes are mostly code style related, I have to ask - is there any official OpenWrt code style guideline or is it just your feeling here? :)
I'm asking because, in my experience, when people start enforcing code style that's based only on their feeling, sooner or later there will be someone who has a different feeling about it. That would make changes like that quite pointless.
rust-lang/Makefile
Outdated
| PKG_VERSION := 1.60.0 | ||
| PKG_RELEASE := 1 | ||
| PKG_NAME:=rust-lang | ||
| PKG_VERSION:=1.24.3 |
There was a problem hiding this comment.
I find this quite confusing. The package is responsible for installing Rust, so the version here should be 1.60.0 (i.e. the version of Rust). 1.24.3 is just a version of rustup. That's a tool for downloading and managing local Rust toolchain.
The thing is that whenever there is a new version of Rust, we might release a new version of this package and the version of the package should reflect the version of Rust being installed.
There was a problem hiding this comment.
Hmm, interesting. Yes, you were saying those different in some PR/issue, but I find this still confusing how it is done. I will rework it to be clear to anyone, no problem at all.
|
Regarding the code style) Yes, you are right that my changes are related to it, but with the things how it goes, I think my colleague is in touch with you, and there were some improvements that you are not using the host system directory anymore to install rust, but host folder in a build system. Even though there should be more changes, I think we can not do anything better right now as first Rust needs to be added to OpenWrt, in your case, and if it is not there, we can just make it work somehow for a few routers even in a hacky way and see if anyone uses it. Also, I did not look at it further as there will be more changes because currently, Turris 1.x routers are not supported, and it fails the entire build for it. I did not submit those changes yet as I don't want to commit changes any other except formal things, which I did not test. Good question about official guidelines or not, and yes, I can confirm the same behavior from my experience. I would say that there are no official guidelines and you will need to force users to let them know about it by using CI, but many packages in the same-named repository were improved over the past, and since that, they have been trying to stick with it. For reference: openwrt/packages#9682 |
I can fix the build but I don't have the device to test it. |
|
Do you have any comments on the package version (see my comment above)? |
|
@BKPepe since these are mostly cosmetic changes, I'd like to merge it into Could you resolve the rust-lang package version and finish the draft, so that we can merge this PR? |
BKPepe
force-pushed
the
polishing
branch
4 times, most recently
from
450d99a to
1062cef
Compare
- Download the tarball from GitHub as their releases are tagged It is not necessary to clone sources. - Makefile changes and reorder things to keep it up with most Makefiles in OpenWrt packages repository, e.g., explicitly state that rust-lang is the host package, it is possible to build it in parallel, etc. - Fix SPDX License Identifiers Also, it changes the approach from CONFIG_TARGET_PROFILE, because this was done only for one device, there is a possibility to use CONFIG_ARCH, which is taken from target.mk, thus anyone with ARMv7 device can use this in OpenWrt build system. It was working for aarch64 before, thus it is not necessary to explicitly mention it here, but rather be safe.
These spaces were caused by text editor, but many OpenWrt Makefiles does not have it, so remove them.
BKPepe
force-pushed
the
polishing
branch
2 times, most recently
from
e1e4a7d to
277964e
Compare
Yes, I had, but I did the changes for you, so it now should be clear how it should be done for OpenWrt build system because you can not use PKG_VERSION and PKG_SOURCE_VERSION at the same time, which results that both versions being different. You were downloading and checking out the rust-up repository for version 1.24.3 and not 1.6.0. I can extend the commit messages, if you want.
You don't need to have the device to test whether it compiles. At least your feed does not crash the entire build anymore. We should move this discussion to #7 My pull request is now compile tested for Turris Omnia and Turris MOX. |
This approach is going to be simpler and faster, because it does not require to have OpenWrt build system. If you want to update your package because you can download the tarball, check the checksum and put it to the correct variable. If the hash does not match, then build system does not proceed anymore to compile this package. In the previous solution, you need to have a ready OpenWrt build system, check the PKG_MIRROR_HASH, and even if it is incorrect, then it is still going to proceed and checkout the sources [1]. [1] https://forum.openwrt.org/t/what-pkg-mirror-hash-or-pkg-hash-means/1602/2
There was a problem hiding this comment.
Thank you for the update. However, I'm still not happy with the PKG_VERSION. As I mentioned before, the package does not install Rusup, it installs Rust. Rustup is just a simple tool, for installing/updating Rust and the related ecosystem. Therefore, PKG_VERSION should be a version of Rust here (see the proposed changes).
If we used the Rustup version here, it would be like creating a Python package but instead of putting a Python version into the PKG_VERSION, we'd be using a pip version.
Alternatively, we could split this package into two. One would be responsible for installing Rustup and the other would be responsible for installing Rust (using Rustup).
| PKG_VERSION := 1.60.0 | ||
| PKG_RELEASE := 1 | ||
| PKG_NAME:=rust-lang | ||
| PKG_VERSION:=1.25.1 |
There was a problem hiding this comment.
| PKG_VERSION:=1.25.1 | |
| PKG_VERSION:=1.60.0 |
There was a problem hiding this comment.
No, this is not correct as I pointed out. You are trying to pursue building system and its variables to do something different than it is used to. If you take a look at OpenWrt documentation, you see that the version there is used to download it and that's what it should be in PKG_SOURCE and PKG_SOURCE_URL. There should not be a hard-coded version, but PKG_VERSION.
You might also check this out https://github.com/mwarning/openwrt-examples
There was a problem hiding this comment.
I understand but the current solution isn't correct either (semantically). Would you be comfortable with splitting this package into two as I proposed (one package for Rustup and one for Rust)?
There was a problem hiding this comment.
Because we are not able to agree on my solution and I don't like yours! 😆 Then, yeah, splitting the package into two sounds reasonable.
There was a problem hiding this comment.
Perfect! 😄
Then I propose the following:
- I will merge this PR as it is.
- I will merge also Fix the Rust install dir and simplify the angelcam-connector Makefile #5.
- I will create a new PR splitting this package into two.
| PKG_SOURCE_URL:=https://github.com/rust-lang/rustup.git | ||
| PKG_SOURCE_VERSION:=1.24.3 | ||
| PKG_MIRROR_HASH:=ba8ebb610aaf76127a475ae899dee44e57e81341965f37b3c534196c933fc2df | ||
| PKG_SOURCE:=rustup-$(PKG_VERSION).tar.gz |
There was a problem hiding this comment.
| PKG_SOURCE:=rustup-$(PKG_VERSION).tar.gz | |
| PKG_SOURCE:=rustup-1.25.1.tar.gz |
| PKG_SOURCE_VERSION:=1.24.3 | ||
| PKG_MIRROR_HASH:=ba8ebb610aaf76127a475ae899dee44e57e81341965f37b3c534196c933fc2df | ||
| PKG_SOURCE:=rustup-$(PKG_VERSION).tar.gz | ||
| PKG_SOURCE_URL:=https://codeload.github.com/rust-lang/rustup/tar.gz/$(PKG_VERSION)? |
There was a problem hiding this comment.
| PKG_SOURCE_URL:=https://codeload.github.com/rust-lang/rustup/tar.gz/$(PKG_VERSION)? | |
| PKG_SOURCE_URL:=https://codeload.github.com/rust-lang/rustup/tar.gz/1.25.1? |
| define Host/Compile | ||
| sh $(HOST_BUILD_DIR)/rustup-init.sh -y \ | ||
| --default-toolchain $(PKG_VERSION) \ | ||
| --default-toolchain 1.60.0 \ |
There was a problem hiding this comment.
| --default-toolchain 1.60.0 \ | |
| --default-toolchain $(PKG_VERSION) \ |
|
Sounds good.
Dne po 15. 8. 2022 11:55 uživatel Ondřej Perutka ***@***.***>
napsal:
… ***@***.**** commented on this pull request.
------------------------------
In rust-lang/Makefile
<#6 (comment)>
:
> @@ -1,24 +1,37 @@
include $(TOPDIR)/rules.mk
-PKG_NAME := rust-lang
-PKG_LICENSE := Apache-2.0/MIT
-PKG_VERSION := 1.60.0
-PKG_RELEASE := 1
+PKG_NAME:=rust-lang
+PKG_VERSION:=1.25.1
Perfect! 😄
Then I propose the following:
1. I will merge this PR as it is.
2. I will merge also #5
<#5>.
3. I will create a new PR splitting this package into two.
—
Reply to this email directly, view it on GitHub
<#6 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA7IDVEYH6GD3XQUKOM6HBLVZIHYNANCNFSM5YCPJJ5A>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
…#5) * Fix the Rust install dir and simplify the angelcam-connector Makefile Rust will be installed within the build tree (the default location used to be user's home). As a side-result of this change, some variables common for rust-lang and angelcam-connector have been moved to rust-lang/files/rust-env.mk. This file contains also common initialization for Rust builds that can further simplify Makefiles for Rust apps. * CR fixes * Makefile(s) polishing (#6) * angelcam-connector: Makefile polishing - Makefile reordering to be more in sync with OpenWrt packages feed - Sync indentation somewhere were used spaces, somewhere not - Reordered install section to be more readable * rust-lang: download tarball from GitHub instead of cloning it - Download the tarball from GitHub as their releases are tagged It is not necessary to clone sources. - Makefile changes and reorder things to keep it up with most Makefiles in OpenWrt packages repository, e.g., explicitly state that rust-lang is the host package, it is possible to build it in parallel, etc. - Fix SPDX License Identifiers Also, it changes the approach from CONFIG_TARGET_PROFILE, because this was done only for one device, there is a possibility to use CONFIG_ARCH, which is taken from target.mk, thus anyone with ARMv7 device can use this in OpenWrt build system. It was working for aarch64 before, thus it is not necessary to explicitly mention it here, but rather be safe. * reforis-angelcam-connector-plugin: remove additional spaces These spaces were caused by text editor, but many OpenWrt Makefiles does not have it, so remove them. * angelcam-connector: download tarball from GitHub This approach is going to be simpler and faster, because it does not require to have OpenWrt build system. If you want to update your package because you can download the tarball, check the checksum and put it to the correct variable. If the hash does not match, then build system does not proceed anymore to compile this package. In the previous solution, you need to have a ready OpenWrt build system, check the PKG_MIRROR_HASH, and even if it is incorrect, then it is still going to proceed and checkout the sources [1]. [1] https://forum.openwrt.org/t/what-pkg-mirror-hash-or-pkg-hash-means/1602/2 Co-authored-by: Josef Schlehofer <[email protected]>
This is based on PR #5
A rather few formal things, which are not tested, yet. Thus PR is marked as draft until the test will be perfomed if it is alright.