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

Update synocli net #4195

Merged
merged 11 commits into from
Oct 11, 2020
Merged

Update synocli net #4195

merged 11 commits into from
Oct 11, 2020

Conversation

hgy59
Copy link
Contributor

@hgy59 hgy59 commented Sep 28, 2020

Motivation: Revise outdated synocli-net packages and add arp-scan
Linked issues: #3742, #4196, closes #3519

Checklist

  • Build rule all-supported completed successfully
  • Package upgrade completed successfully
  • New installation of package completed successfully

Remarks

  • add diyspk for some packages of synocli-net to verify updates before building the collection
  • unfortunately with newer toolchains added, the rule all-supported does not use the TC 6.1 anymore but takes the newest TC available. Therefore I created the rule all-general to build TC 6.1 and ppc853x-5.2 version.
  • most of the contained packages depend on cross/openssl and my intention is to merge this to master (and not to publish) before the update of openssl to version 1.1.x.
  • dependency of Perl is removed and mosh is moved to a dedicated package
  • add arp-scan (includes three perl scripts that are not mandatory and therefore SPK_DEPENDS=Perl is omitted
  • add links and remove dedicated package (moved to diyspk)

@hgy59
Copy link
Contributor Author

hgy59 commented Sep 29, 2020

rutorrent still fails to build (depending on updated screen) but I didn't find a fix so far.
So I propose to merge this PR to go further with openssl update.

@th0ma7
Copy link
Contributor

th0ma7 commented Sep 29, 2020

Hi @hgy59 , a few questions for you:

unfortunately with newer toolchains added, the rule all-supported does not use the TC 6.1 anymore but takes the newest TC available. Therefore I created the rule all-general to build TC 6.1 and ppc853x-5.2 version.

  • Doesn't a make setup forces to use DSM6.1 thus building package as before? Or should the make all-supported rule set be reviewed as well?
  • Indeed the openssl.cnf is not installed anymore with the usage of /etc/ssl by default. What was the reason to revert the change that you first did? And should it apply to newer 1.1.1 version?
  • For my own understanding what does the diyspk stands for?

@hgy59
Copy link
Contributor Author

hgy59 commented Sep 29, 2020

  • Doesn't a make setup forces to use DSM6.1 thus building package as before? Or should the make all-supported rule set be reviewed as well?

make setup sets the default TC to DSM 6.1, but this is only for make arch-{arch} without specification of the TC version.
make all-supported does not regard the DSM version set with make setup - it takes the highest version available for each arch. I don't know why the 6.2 and 6.2.2 toolchains where added. We would need the exact version only for packages that require the kernel sources (afaik). Otherwise we (should) publish with the lowest version compatible with the actual version.

May be it is worth to remove my all-general target and adjust the all-supported target to work like this. ❓

Another approach would be to regard the default TC version set with make setup - but with auto resolving the version when the exact version is not available. i.e. take the highest TC version of the arch if it is less than the default version - or - take the lowest TC version if versions greater than the default are available.

Other considerations:

  • make all-legacy builds still for TC 4.3
  • make all-supported should include the build for ppc853x-5.2 as this is a still supported version by synology.com.
  • I suppose that with DSM 7 we will have to build for 6.1 and 7.0 in parallel, as some models will not get the update to DSM 7 (as DS210+ did not for DSM 6)

  • Indeed the openssl.cnf is not installed anymore with the usage of /etc/ssl by default. What was the reason to revert the change that you first did? And should it apply to newer 1.1.1 version?

I removed the openssl.cnf because it is not used anymore and gives a warning due to missing the file. And I reverted it as this is not related to synocli-net and forces all the openssl dependent packages to be built by the github build action. (it sould be removed from the PLIST in a PR related to openssl)


  • For my own understanding what does the diyspk stands for?

This stands for do it your self packages, i.e. packages that can be built by developpers themself and that are not published (any more) to the SynoCommunity package repo. It was introduced with the creation of synocli-* package collections. Since the synocli collections contain a lot of packages it is very useful to have the single package diyspk versions to build and test updated packages that are part of synocli-*.

@th0ma7
Copy link
Contributor

th0ma7 commented Sep 29, 2020

@hgy59 thnx for the answers, additional comments on it:

  1. Agreed, now that I better understand the behaviour of all-supported I believe this really this should map to what we do support which should range from ppc853x-5.2 up to DSM-6.1 (optionnally new 6.2 only arches such as purley, geminilake, etc). This will need to be readjust later anyway with the arrival of DSM7. With a similar mindset, all-legacy should be revisited as well but its not urgent for the moment (perhaps with DSM7). And yes 6.2 & 6.2.2 where added due to kernel related requirements. Sadly 6.2.3 was never published by Synology which contains all the intel cpu security fixes that where backported. This leads to module & kernel crashes at loading time when loading a 6.2.2 compiled module under a 6.2.3 running kernel...
  2. Good, I will ensure its there in the openssl PR (lack of time this week to look into it but I believe I already removed it)
  3. Really interesting I didn't know... there may be some interest for sub ffmpeg packages like intel drivers for hardware acceleration for instance. I'll keep that to mind.

@th0ma7 th0ma7 mentioned this pull request Oct 1, 2020
3 tasks
@th0ma7 th0ma7 mentioned this pull request Oct 10, 2020
55 tasks
Copy link
Contributor

@th0ma7 th0ma7 left a comment

Choose a reason for hiding this comment

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

Why removing sshfs? ... just re-read and figure you are not removing sshfs but rather moving it as diyspk instead of standalone spk but also keeing in into synocli-net, I'm all good witht hat.

But shouldn't the make all-general be superseeded with #4200 by now?

Besides that this changeset looks good to me.

@ymartin59
Copy link
Contributor

@hgy59 @th0ma7 I would like to avoid to create "some many" diyspk/ packages because it increases "maintenance cost" when there is "no need" or no obvious tips they will be used by anyone sooner-or-later.
First "diyspk" packages were introduced to keep previous "standalone package" for small applications still available (not to enforce users to change if able to build on their own), when introducing aggregation in "synocli" packages...

Notice that in previous discussions, I agree to extract "mosh" out of "synocli-net" as a standalone package spk/mosh because it is quite heavy and introduces dependency to Python. May you please move diyspk/mosh to spk/mosh and cut dependencies to make synocli-net far lighter.

Copy link
Contributor

@ymartin59 ymartin59 left a comment

Choose a reason for hiding this comment

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

I would also move spk/sshfs to diyspk/sshfs for "compatibility" only

cross/arp-scan/patches/001-update-get-iab.patch Outdated Show resolved Hide resolved
mk/spksrc.common.mk Outdated Show resolved Hide resolved
mk/spksrc.spk.mk Outdated Show resolved Hide resolved
@ymartin59
Copy link
Contributor

According to this diyspk "migration" policy, it would make sense to remove them "one day". But again I would not create diyspk package for so small new tools.

@hgy59
Copy link
Contributor Author

hgy59 commented Oct 10, 2020

According to this diyspk "migration" policy, it would make sense to remove them "one day". But again I would not create diyspk package for so small new tools.

I find it rather usefull to have such diyspk for the synocli-* collections and would like to keep them, as you can build and test single package updates without the need to builld and install the whole collection.

@ymartin59
Copy link
Contributor

@hgy59 OK. For sure if they are used for testing, there is less chance to forget to update them. We will see... until then go on that way.
About mosh own package, would it be possible for you to take over what I started at #3820 (of course forget about Meson support which will be addressed later)
There is also a trouble to investigate with mosh (and probably tmux) concerned detached session: #3582 #3583
Again many thanks for your work here.

@hgy59
Copy link
Contributor Author

hgy59 commented Oct 10, 2020

I propose to rebase and merge only after #4155 is merged into master.

@hgy59
Copy link
Contributor Author

hgy59 commented Oct 10, 2020

@ymartin59 build fails due to error in openssl build (1.0.1u).
So I definitly wait for the merge of #4155 before finalizing this.

The error comes form installing openssl parts into /var/packages instead of using the install folder ../install/var/packages/synocli-net/target/...
I propose to forget openssl 1.0.1u and focus on 1.1.1.

@ymartin59
Copy link
Contributor

@hgy59 The fix quite simple. I propose you grab it from my PR #4195 to confirm

$(RUN) $(MAKE) install_sw INSTALL_PREFIX=$(INSTALL_DIR)

@hgy59
Copy link
Contributor Author

hgy59 commented Oct 10, 2020

@hgy59 The fix quite simple. I propose you grab it from my PR #4195 to confirm

$(RUN) $(MAKE) install_sw INSTALL_PREFIX=$(INSTALL_DIR)

Yes this is the fix, but commiting would lead to merge conflicts - therefore I proposed to forget about openssl 1.0.1u.

@hgy59 hgy59 mentioned this pull request Oct 10, 2020
3 tasks
@ymartin59
Copy link
Contributor

@hgy59 No there will be no merge conflict with Git if change is stricly the exact same line, that is why it is worth trying.

@hgy59 hgy59 mentioned this pull request Oct 11, 2020
30 tasks
- update screen to version 4.8.0
- update socat to version 1.7.3.4
- update nmap to version 7.80
- add arp-scan (SynoCommunity#3519)
- add diyspk for screen, socat, sshfs, nmap, arp-scan
- remove openssl.cnf from openssl/PLIST as it is not installed anymore (see SynoCommunity#4192)
- add spk target all-general to build the same arch-tc combinations as github build action does
- tmux is part of synocli-net
- tmux is not published as dedicated package anymore
- sshfs is part of synocli-net
- sshfs is not published as dedicated package
- sshfs is already provided as diyspk
- include links in synocli-net
- move spk/links to diyspk/links
@hgy59 hgy59 merged commit a3c6f76 into SynoCommunity:master Oct 11, 2020
@hgy59 hgy59 deleted the update_synocli-net branch October 25, 2020 12:45
@hgy59 hgy59 added the status/published Published and activated (may take up to 48h until visible in DSM package manager) label Oct 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/published Published and activated (may take up to 48h until visible in DSM package manager)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Arp-scan for synology?
3 participants