-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
openwisp-config: fix Makefile for 1.1.0 #25186
base: master
Are you sure you want to change the base?
Conversation
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.
LGTM ✔️
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.
@feckert we fixed some issues and updated some outdated code here.
Any maintainer could merge this please? @BKPepe |
$(INSTALL_BIN) \ | ||
$(PKG_BUILD_DIR)/openwisp-config/files/openwisp.agent \ | ||
$(1)/usr/sbin/openwisp_config | ||
$(1)/usr/sbin/openwisp-config | ||
|
||
$(INSTALL_BIN) \ | ||
$(PKG_BUILD_DIR)/openwisp-config/files/openwisp.init \ | ||
$(1)/etc/init.d/openwisp_config | ||
$(1)/etc/init.d/openwisp-config |
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.
While checking the commit description, this one is only valid, isnt it? :)
$(PKG_BUILD_DIR)/openwisp-config/files/sbin/openwisp-get-random-number.lua \ | ||
$(1)/usr/sbin/openwisp-get-random-number | ||
|
||
$(CP) $(PKG_BUILD_DIR)/VERSION $(1)/usr/lib/openwisp-config/ |
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 this is necessary?
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's used by the program to read the version number
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 did not understand this commit.
admin/openwisp-config/Makefile
Outdated
define Package/openwisp-config/postrm | ||
#!/bin/sh | ||
rm -f $${IPKG_INSTROOT}/usr/sbin/openwisp_config | ||
endef |
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.
This will be removed automatically. No needed.
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.
The agent is now named /usr/sbin/openwisp-config
, we create a symbolic link to /usr/sbin/openwisp_config
for backward compatibility. This postrm hook removes the symbolic link. Was this clear?
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 know what you would like to achieve, but I am asking why do it in such complicated way.
admin/openwisp-config/Makefile
Outdated
# for backward compatibility | ||
define Package/openwisp-config/postinst | ||
#!/bin/sh | ||
if [ ! -L $${IPKG_INSTROOT}/usr/sbin/openwisp_config ]; then | ||
ln -s /usr/sbin/openwisp-config $${IPKG_INSTROOT}/usr/sbin/openwisp_config | ||
fi | ||
endef |
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.
Also, this one, well, its breaking change introduced by previous version. Thus users will run this package by init script, right? I find this useless.
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 guess we can remove it. Initially we copied the init script too but it caused to have multiple agents running.
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.
@BKPepe removed
Update configuration in Makefile to fix openwrt#25168 Signed-off-by: Gagan Deep <pandafy.dev@gmail.com>
0a9b79f
to
b3940fa
Compare
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.
Ready for review.
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.
Any blocker here?
@BKPepe Do you have any comments or can I merge this? |
Update configuration in Makefile to fix #25168
Maintainer: @nemesifier
Compile tested: ramips
Run tested: ramips