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

subversion: polish Makefile #9667

Merged
merged 1 commit into from Aug 14, 2019
Merged

subversion: polish Makefile #9667

merged 1 commit into from Aug 14, 2019

Conversation

ja-pa
Copy link
Contributor

@ja-pa ja-pa commented Aug 6, 2019

Maintainer: @val-kulkov
Compile tested: N/A
Run tested: N/A

Description:
This PR adds PKG_CPE_ID to Makefile variables since version 1.12.2 fixed two security issues
Release note and polishes Makefile similarly to #9666

Changes:
Polish variable order
Add PKG_CPE_ID

Signed-off-by: Jan Pavlinec jan.pavlinec@nic.cz

@ja-pa ja-pa marked this pull request as ready for review August 6, 2019 13:36
@val-kulkov
Copy link
Contributor

I am okay with adding PKG_CPE_ID and with replacing tabs with spaces in "define Package/subversion/Default/description". I see no added value in changing the order of variables or merging a double-liner into a runaway one-liner. IMO such changes only increase the size of the Git repo and pollute the source code history.

Until OpenWrt publishes guidelines on the order of variables in Makefile, I am against making such changes that bring little or no value while increasing the size of the repo.

@ja-pa
Copy link
Contributor Author

ja-pa commented Aug 6, 2019

@val-kulkov OK, I can remove those changes. It seemed logical to me to squeeze that changes into one PR, so stable and master package versions are more similar.

When it comes to variable order, I'm trying to stick with suggested template from @BKPepe #9399 (comment)

@val-kulkov
Copy link
Contributor

@ja-pa : I am not saying that I know better. IMHO the clean code history and the size of the repo are important. IMHO in this particular case, they outweigh the benefit of making the master Makefile and the LTS Makefile more similar.

@neheb
Copy link
Contributor

neheb commented Aug 6, 2019

Why does the size matter? ./scripts/feeds/update makes a shallow clone.

@val-kulkov
Copy link
Contributor

@neheb : indeed it does not matter if your goal is to compile a package. But if your goal is to review code history, it does matter. You cannot review code history from a shallow clone. Besides, IMO a non-functional re-arrangement of lines is a distraction that makes the code more time-consuming to review.

@neheb
Copy link
Contributor

neheb commented Aug 8, 2019

OTOH it makes it more consistent with other packages. Makes for easier reading between them.

@ja-pa
Copy link
Contributor Author

ja-pa commented Aug 9, 2019

@val-kulkov Ok, so I revert variable order changes and create an issue where we could continue with discussion #9682

@val-kulkov
Copy link
Contributor

One last thing that many of us tend to forget about: PKG_RELEASE. Please increment PKG_RELEASE and then everything should be good to go. Thank you.

@ja-pa ja-pa force-pushed the subversion-cpe-id branch 2 times, most recently from 79f1dd0 to 9a092ef Compare August 14, 2019 09:46
@ja-pa
Copy link
Contributor Author

ja-pa commented Aug 14, 2019

@val-kulkov Updated.

Changes:
Description replace tabs
Add PKG_CPE_ID

Signed-off-by: Jan Pavlinec <jan.pavlinec@nic.cz>
@val-kulkov
Copy link
Contributor

LGTM.

@neheb neheb merged commit 746935f into openwrt:master Aug 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants