luci-app-ntpc: remove app #6945

dannil · 2024-02-28T17:54:47Z

As ntpclient package is dead, we can delete the app.

See #6945 (comment) and openwrt/packages#23569

Converted the app to use client-side JavaScript instead of Lua. The only real change to the previous implementation is I changed the Current system time field to match the format in Status -> System -> Local Time as I could not get the %c date format to work with the format function; if this is possible feel free to comment this. Package doesn't have a PKG_VERSION so by definition it hasn't been incremented.

systemcrash · 2024-02-28T18:09:16Z

Great! I didn't even notice we had this app.

Could you hint the host name and port fields for when they're empty?

Maybe also add in the package description that this is a client.

systemcrash · 2024-02-28T18:12:02Z

Also, would it be interesting to try and calculate the latency between browser and openwrt and attempt to compensate for the offset? Code golf perhaps 😎

Entirely optional.

dannil · 2024-02-28T18:21:14Z

Great! I didn't even notice we had this app.

Could you hint the host name and port fields for when they're empty?

Maybe also add in the package description that this is a client.

That sounds like good additions, I will add that.

Also, would it be interesting to try and calculate the latency between browser and openwrt and attempt to compensate for the offset? Code golf perhaps 😎

Entirely optional.

Haha, I can give it a shot, but I don't promise anything, if anything I can experiment with it and open a PR later on for it.

dannil · 2024-02-28T18:59:14Z

@systemcrash I've updated the code with your suggestions.

applications/luci-app-ntpc/Makefile

systemcrash · 2024-03-01T18:12:18Z

One question: the local Date() clock monotonically increases with time?

dannil · 2024-03-01T18:31:28Z

One question: the local Date() clock monotonically increases with time?

No, once you enter the app, the clock fetches the current timestamp and displays that, with no auto-refresh or anything fancy like that. That is also how the previous implementation in Lua behaved so I didn't change that, but it wouldn't be hard to implement this if we wanted, like how the overview page auto-refreshes the Local Time every 5 seconds.

systemcrash · 2024-03-01T19:34:30Z

OK - I'm trying to reconcile what System -> Time Sync does not do. With such similar data and function, perhaps the two could be merged if they are not already the same.

dannil · 2024-03-01T19:52:47Z

OK - I'm trying to reconcile what System -> Time Sync does not do. With such similar data and function, perhaps the two could be merged if they are not already the same.

I think the biggest difference is that System -> Time Sync is built to support ntpd while this app was built to support ntpclient, so consolidating them would be quite a challenge of the top of my head as we would need to adapt luci-mod-system to support configuring multiple NTP daemons.

We could of course think of adding UI options of features that ntpclient has that ntpd doesn't but that it currently supports but I think that's entirely separate PR:s to do that work.

systemcrash · 2024-03-01T20:24:03Z

So I just had a look at the ntpclient package. It points to http://doolittle.icarus.com/ntpclient which is dead.

Unless a suitable replacement is found, the package can be removed. The existing system can make calls to other ntpd sources to synchronise, which is basically what ntpclient does, right?

dannil · 2024-03-01T20:51:48Z

Nice find, I didn't even know that the project was dead. I'm not opposed to just deleting the whole package, I don't know what the processes are regarding that if there's some users that are currently relying on it?

The existing system can make calls to other ntpd sources to synchronise, which is basically what ntpclient does, right?

I'm not entirely sure but yes I think so. I created this PR purely to port luci-app-ntpc to JavaScript and what I did to verify was to check that the UCI config values that the Lua implementation wrote to was still updated correctly after the conversion.

$ uci show ntpclient
ntpclient.@ntpserver[0]=ntpserver
ntpclient.@ntpserver[0].hostname='0.openwrt.pool.ntp.org'
ntpclient.@ntpserver[0].port='123'
ntpclient.@ntpserver[1]=ntpserver
ntpclient.@ntpserver[1].hostname='1.openwrt.pool.ntp.org'
ntpclient.@ntpserver[1].port='123'
ntpclient.@ntpserver[2]=ntpserver
ntpclient.@ntpserver[2].hostname='2.openwrt.pool.ntp.org'
ntpclient.@ntpserver[2].port='123'
ntpclient.@ntpserver[3]=ntpserver
ntpclient.@ntpserver[3].hostname='3.openwrt.pool.ntp.org'
ntpclient.@ntpserver[3].port='123'
ntpclient.@ntpdrift[0]=ntpdrift
ntpclient.@ntpdrift[0].freq='0'
ntpclient.@ntpclient[0]=ntpclient
ntpclient.@ntpclient[0].interval='600'

systemcrash · 2024-03-01T21:11:31Z

Nice find, I didn't even know that the project was dead. I'm not opposed to just deleting the whole package, I don't know what the processes are regarding that if there's some users that are currently relying on it?

Not sure how anyone can rely on a package which has no sources any longer. We can check back in a few days and see whether the host comes back.

I've made a PR otherwise here openwrt/packages#23569

because

stokito

Nice work.
Please become a maintainer and add PKG_MAINTAINER

stokito · 2024-03-03T11:03:49Z

applications/luci-app-ntpc/po/sk/ntpc.po

-#: applications/luci-app-ntpc/luasrc/model/cbi/ntpc/ntpcmini.lua:6
-msgid "Synchronizes the system time"
+#: applications/luci-app-ntpc/htdocs/luci-static/resources/view/ntpc.js:24
+msgid "Synchronizes the system time."


Please remove the dot at end and this will keep a translation

stokito · 2024-03-03T11:06:03Z

applications/luci-app-ntpc/htdocs/luci-static/resources/view/ntpc.js

+
+return view.extend({
+	load: function () {
+		return Promise.all([


Here you have only one promise so you can remove the Promise.all()

stokito · 2024-03-03T11:09:47Z

applications/luci-app-ntpc/htdocs/luci-static/resources/view/ntpc.js

+	render: function (data) {
+		var system_info = data[0];
+
+		var m, s1, time, interval, count, freq, s2, s3, hostname, port;


Usually for section and option separate vars aren't created.

stokito · 2024-03-03T11:10:30Z

applications/luci-app-ntpc/htdocs/luci-static/resources/view/ntpc.js

+
+		m = new form.Map('ntpclient', _('Time Synchronisation'), _('Synchronizes the system time.'));
+
+		s1 = m.section(form.TypedSection, 'ntpclient', _('General'));


So instead of s1 use:
s = m.section(form.TypedSection, 'ntpclient', _('General'));

stokito · 2024-03-03T11:11:04Z

applications/luci-app-ntpc/htdocs/luci-static/resources/view/ntpc.js

+			);
+		}
+
+		time = s1.option(form.DummyValue, '_time', _('Current system time'));


o = s.option(form.DummyValue, '_time', _('Current system time'));

dannil · 2024-03-03T11:20:46Z

Nice work. Please become a maintainer and add PKG_MAINTAINER

@systemcrash has a PR open in openwrt/packages#23569 to drop support for ntpclient since the sources seems to be dead (#6945 (comment)), so we may drop the app entirely instead.

If the decision is made to drop it, I can just close this PR and create a new one which drops the app. Otherwise I can add myself as a maintainer.

stokito · 2024-03-03T22:52:08Z

Interesting, I found that Luci already has NTP config in a System page if the sysntpd from BusyBox was enabled during a compilation.

Its settings are stored to /etc/config/system:

config timeserver 'ntp'
    option enabled '1'
    option enable_server '0'
    list server '0.openwrt.pool.ntp.org'
    list server '1.openwrt.pool.ntp.org'

So yes, probably the feature may be removed.
Still it may be used in some old firmware of some vendors. So maybe the app will be useful for them.
When removing the app please tag the issue so they may find the PR and maybe use it once last time.

hnyman · 2024-03-04T05:38:28Z

Busybox ntp has been the default server and client at least for a decade, and there has been config for that in System settings (luci-mod-system) all the time.

So, I really wonder if somebody is actively using this. This app has not been modified for some 10 years...
@dannil
you got interested in this. do you actually use it?

dannil · 2024-03-04T18:53:01Z

Busybox ntp has been the default server and client at least for a decade, and there has been config for that in System settings (luci-mod-system) all the time.

So, I really wonder if somebody is actively using this. This app has not been modified for some 10 years... @dannil you got interested in this. do you actually use it?

No I don't use it, I basically picked this as a "test platform" to get into developing in LuCI (the whole "we don't develop apps in Lua anymore" seemed a good issue to start with). I'm fine to drop support for the whole app it if we conclude that the ntp functionality in System settings fulfill the same goals.

systemcrash · 2024-03-05T13:32:18Z

OK, we can axe ntpc.

@dannil please truncate this PR just to removal.

As ntpclient package is dead, we can delete the app. Signed-off-by: Daniel Nilsson <daniel.nilsson94@outlook.com>

dannil · 2024-03-05T16:30:19Z

PR and commit is updated to remove the app instead.

BKPepe · 2024-03-06T19:52:16Z

It needs to be backported to OpenWrt 23.05.

ntp sources are dead and gone. The most important functionality is now provided by ntpd. Signed-off-by: Paul Donald <newtwen@gmail.com> (cherry picked from commit 2cd10d8)

dannil · 2024-03-06T20:24:09Z

I've opened #6970 to backport it.

luci-app-ntpc: remove app (cherry picked from commit 452e813)

dannil force-pushed the ntpc-to-js branch from 9882e17 to 3d2d2ca Compare February 28, 2024 17:58

dannil force-pushed the ntpc-to-js branch from 3d2d2ca to 984b9a4 Compare February 28, 2024 18:56

dannil commented Feb 28, 2024

View reviewed changes

applications/luci-app-ntpc/Makefile Outdated Show resolved Hide resolved

dannil force-pushed the ntpc-to-js branch from 984b9a4 to ff61fd9 Compare March 1, 2024 19:56

systemcrash previously approved these changes Mar 1, 2024

View reviewed changes

systemcrash marked this pull request as draft March 1, 2024 21:23

stokito approved these changes Mar 3, 2024

View reviewed changes

dannil force-pushed the ntpc-to-js branch from ff61fd9 to 573459d Compare March 3, 2024 11:31

luci-app-ntpc: remove app

6cd54b4

As ntpclient package is dead, we can delete the app. Signed-off-by: Daniel Nilsson <daniel.nilsson94@outlook.com>

dannil force-pushed the ntpc-to-js branch from 573459d to 6cd54b4 Compare March 5, 2024 16:29

dannil changed the title ~~luci-app-ntpc: convert to JavaScript~~ luci-app-ntpc: remove app Mar 5, 2024

dannil marked this pull request as ready for review March 5, 2024 16:30

hnyman merged commit 452e813 into openwrt:master Mar 5, 2024
5 checks passed

BKPepe referenced this pull request in openwrt/packages Mar 6, 2024

ntpclient: remove

6c7b6bf

ntp sources are dead and gone. The most important functionality is now provided by ntpd. Signed-off-by: Paul Donald <newtwen@gmail.com> (cherry picked from commit 2cd10d8)

dannil deleted the ntpc-to-js branch March 6, 2024 20:14

dannil mentioned this pull request Mar 6, 2024

[23.05] luci-app-ntpc: remove app #6970

Merged

systemcrash pushed a commit that referenced this pull request Mar 6, 2024

Merge pull request #6945 from dannil/ntpc-to-js

cd86ec6

luci-app-ntpc: remove app (cherry picked from commit 452e813)

systemcrash pushed a commit that referenced this pull request Mar 7, 2024

Merge pull request #6945 from dannil/ntpc-to-js

821b747

luci-app-ntpc: remove app (cherry picked from commit 452e813)

db260179 pushed a commit to db260179/openwrt-luci that referenced this pull request Mar 16, 2024

Merge pull request openwrt#6945 from dannil/ntpc-to-js

9415f24

luci-app-ntpc: remove app (cherry picked from commit 452e813)

db260179 pushed a commit to db260179/openwrt-luci that referenced this pull request Mar 22, 2024

Merge pull request openwrt#6945 from dannil/ntpc-to-js

7cb9095

luci-app-ntpc: remove app (cherry picked from commit 452e813)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

luci-app-ntpc: remove app #6945

luci-app-ntpc: remove app #6945

dannil commented Feb 28, 2024 •

edited

Loading

systemcrash commented Feb 28, 2024

systemcrash commented Feb 28, 2024

dannil commented Feb 28, 2024

dannil commented Feb 28, 2024 •

edited

Loading

systemcrash commented Mar 1, 2024

dannil commented Mar 1, 2024

systemcrash commented Mar 1, 2024

dannil commented Mar 1, 2024

systemcrash commented Mar 1, 2024

dannil commented Mar 1, 2024 •

edited

Loading

systemcrash commented Mar 1, 2024

stokito left a comment

stokito Mar 3, 2024

dannil Mar 3, 2024

stokito Mar 3, 2024

dannil Mar 3, 2024

stokito Mar 3, 2024

dannil Mar 3, 2024

stokito Mar 3, 2024

dannil Mar 3, 2024

stokito Mar 3, 2024

dannil Mar 3, 2024

dannil commented Mar 3, 2024 •

edited

Loading

stokito commented Mar 3, 2024 •

edited

Loading

hnyman commented Mar 4, 2024

dannil commented Mar 4, 2024 •

edited

Loading

systemcrash commented Mar 5, 2024

dannil commented Mar 5, 2024

BKPepe commented Mar 6, 2024

dannil commented Mar 6, 2024


		m = new form.Map('ntpclient', _('Time Synchronisation'), _('Synchronizes the system time.'));

		s1 = m.section(form.TypedSection, 'ntpclient', _('General'));

luci-app-ntpc: remove app #6945

luci-app-ntpc: remove app #6945

Conversation

dannil commented Feb 28, 2024 • edited Loading

systemcrash commented Feb 28, 2024

systemcrash commented Feb 28, 2024

dannil commented Feb 28, 2024

dannil commented Feb 28, 2024 • edited Loading

systemcrash commented Mar 1, 2024

dannil commented Mar 1, 2024

systemcrash commented Mar 1, 2024

dannil commented Mar 1, 2024

systemcrash commented Mar 1, 2024

dannil commented Mar 1, 2024 • edited Loading

systemcrash commented Mar 1, 2024

stokito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannil commented Mar 3, 2024 • edited Loading

stokito commented Mar 3, 2024 • edited Loading

hnyman commented Mar 4, 2024

dannil commented Mar 4, 2024 • edited Loading

systemcrash commented Mar 5, 2024

dannil commented Mar 5, 2024

BKPepe commented Mar 6, 2024

dannil commented Mar 6, 2024

dannil commented Feb 28, 2024 •

edited

Loading

dannil commented Feb 28, 2024 •

edited

Loading

dannil commented Mar 1, 2024 •

edited

Loading

dannil commented Mar 3, 2024 •

edited

Loading

stokito commented Mar 3, 2024 •

edited

Loading

dannil commented Mar 4, 2024 •

edited

Loading