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

nodogsplash: explicit file copy #1027

Merged
merged 1 commit into from
Oct 23, 2023
Merged

Conversation

mwarning
Copy link
Contributor

Maintainer: me
Compile tested: pipeline

@mwarning
Copy link
Contributor Author

@BKPepe how is this?

@mwarning
Copy link
Contributor Author

@BKPepe can I cherry-pick this into the other MRs for the other branches? Or do you want me to squash them?

@mwarning
Copy link
Contributor Author

@BKPepe ping

@BKPepe
Copy link
Member

BKPepe commented Oct 19, 2023 via email

Comment on lines 51 to 54
$(CP) ./files/etc/config/nodogsplash $(1)/etc/config/
$(CP) ./files/etc/init.d/nodogsplash $(1)/etc/init.d/
$(CP) ./files/etc/uci-defaults/40_nodogsplash $(1)/etc/uci-defaults/
$(CP) ./files/usr/lib/nodogsplash/restart.sh $(1)/usr/lib/nodogsplash/
Copy link
Member

Choose a reason for hiding this comment

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

This still does not look good. We are using simple CP. This needs to be reworked.

We should make it more clear, which will be more better to read this install section and it will be simple as it is.

  1. Create folder, then install files to it and then create another folder and install there files, which are there.
    The order if you do first binaries, then config files, does not matter. Important is that it is not lost.

This is done for binary (a few lines above) and then you are creating three folders and it might be messy.

Anyway, I would suggest to use installation macros, which are listed here:
https://openwrt.org/docs/guide-developer/packages#file_installation_macros

E.g., INSTALL_CONF for config file, INSTALL_BIN for init config, etc.

This makes sure that it has permissions on the router, which we want and that it has not been tinkerered with.

@mwarning mwarning force-pushed the nds_cleanup branch 2 times, most recently from 6ad88ca to 0e50a4d Compare October 22, 2023 21:15
nodogsplash/Makefile Outdated Show resolved Hide resolved
Signed-off-by: Moritz Warning <[email protected]>
@BKPepe BKPepe merged commit 2a725e7 into openwrt:master Oct 23, 2023
11 checks passed
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.

2 participants