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 framework archs #4320

Merged
merged 27 commits into from
Dec 29, 2020
Merged

Update framework archs #4320

merged 27 commits into from
Dec 29, 2020

Conversation

hgy59
Copy link
Contributor

@hgy59 hgy59 commented Dec 19, 2020

Motivation: Improve the ARCH-TC handling and make it more readable
Linked issues: #4200, #4143

Checklist

  • Build rule all-supported completed successfully
    - make all-supported restored to behave as before make all-supported and all-default #4200
  • Remove the x86 toolchains as never used (was just a copy of x86_64 / pineview)
  • Prepare for DSM7 (when syno-x64-7.0 is available make all-supported with DEFAULT_TC=6.1 should build x64-6.1 and not x64-7.0)
  • rename spksrc/toolchains to spksrc/toolchain (open issue of DSM toolkit mk helper #4143)
  • add TC_GCC and TC_GLIBC definitions to toolchain Makefiles (for future UNSUPPORTED package validation).
  • move ppc853x to deprecated archs and remove ppc853x-5.2 from github build action

Remarks

  • outsource the ARCH definitions to mk/spksrc.archs.mk so Makefiles need to include this file for arch specific rules only (before spksrc.common.mk had to be used)
  • *_ARCHES definitions are renamed to *_ARCHS (overall consistent naming)
  • rename x86_ARCHS to i686_ARCHS for x86 32-bit archs (evansport only)
  • introduce ARMv7L_ARCHS for armv7 without hard float (hi3535 only)
  • OLD_PPC_ARCHS is defined as often used for UNSUPPORTED_ARCHS
  • remove the GENERIC_ARCHS handling as we have armv7 generic arch (it was used for cross-go before generic armv7 was fully established)
  • Include some cleanup of make all-supported and all-default #4200
    • exclude generic archs from all-legacy targets
  • use LEGACY_ARCHS now for builds with kernel source required
  • add patch subfolder handling for arch-group with TCVERSION

Wiki variables for review:

  • ARCHS_NO_KRNLSUPP to ARCHS_WITH_KERNEL_SUPPORT

Results

  • make all-supported
    ├── work-88f6281-6.1
    ├── work-aarch64-6.1
    ├── work-armv7-6.1
    ├── work-evansport-6.1
    ├── work-hi3535-6.1
    ├── work-qoriq-6.1
    └── work-x64-6.1

  • make all-legacy
    ├── work-88f6281-5.2
    ├── work-alpine-5.2
    ├── work-armada370-5.2
    ├── work-armada375-5.2
    ├── work-armada38x-5.2
    ├── work-armadaxp-5.2
    ├── work-avoton-5.2
    ├── work-braswell-5.2
    ├── work-bromolow-5.2
    ├── work-cedarview-5.2
    ├── work-comcerto2k-5.2
    ├── work-dakota-1.2
    ├── work-evansport-5.2
    ├── work-ipq806x-1.2
    ├── work-monaco-5.2
    ├── work-northstarplus-1.2
    ├── work-ppc853x-5.2
    ├── work-qoriq-5.2
    └── work-x86-5.2

  • make all-latest
    ├── work-88f6281-6.2.3
    ├── work-aarch64-6.2.3
    ├── work-armv7-6.2.3
    ├── work-evansport-6.2.3
    ├── work-hi3535-6.2.3
    ├── work-qoriq-6.2.3
    └── work-x64-6.2.3

@hgy59 hgy59 requested a review from th0ma7 December 20, 2020 11:11
@hgy59 hgy59 changed the title [WIP] Update framework archs Update framework archs Dec 20, 2020
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.

This is looking good.
Going through the code why not drop entirely the S ?
With that in mind, toolchains is still something that would need renaming to toolchain ...
Overall a lot of comments for consideration. Not that they all need to be actionned (some may be for discussion or for a later PR)

@th0ma7
Copy link
Contributor

th0ma7 commented Dec 22, 2020

Wow this is massive: close to 500 file changed... (most of it due to the toolchain renaming most probably).
Allow me a couple of days to fully review that. At first glance this is looking good.

@th0ma7
Copy link
Contributor

th0ma7 commented Dec 22, 2020

It seems somehow my intent was to propose a change but it got pushed in... no wonder why I prefer command lines to web interface... sigh. Sorry for that.

I think you'll get what the intent is. As we are often using the exact same combination of "LEGACY" archs that a flagged as unsupported might be worth flagging them as legacy?

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.

So last bit of comments:

  • Overall looking quite good
  • Really much like the reworking of the archs.mk, should be more flexible and easier to maintain
  • Much like the new patch/arch-DSMVER.patch funcionality. Should help for DSM7 migration.

Proposals:

  • Rename OLD_PPC_ARCHS into LEGACY_PPC_ARCHS
  • Add a LEGACY_ARCHS that might include ARM5 + potentially also hi3535
  • Document the GENERIC_ARCHS which is I recall had an implication over GO related builds

@th0ma7 th0ma7 mentioned this pull request Dec 22, 2020
9 tasks
@th0ma7
Copy link
Contributor

th0ma7 commented Dec 22, 2020

Quick question, could the github-action be modified so hi3535 builds are done? perhaps instead of armv7-1.2 for instance thus aligning the all-supported further with the github-action.

@hgy59
Copy link
Contributor Author

hgy59 commented Dec 22, 2020

Quick question, could the github-action be modified so hi3535 builds are done? perhaps instead of armv7-1.2 for instance thus aligning the all-supported further with the github-action.

I wanted to suggest that too 👍

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 not rename x86 to pineview so it is consistent?

@@ -59,7 +59,7 @@ pre_wheel_target: wheel_msg_target

build_wheel_target: $(PRE_WHEEL_TARGET)
@if [ ! -z "$(WHEELS)" ] ; then \
$(foreach e,$(shell cat $(WORK_DIR)/python-cc.mk),$(eval $(e))) \
$(foreach e,$(shell cat $(WORK_DIR)/python-cc.mk 2> /dev/null),$(eval $(e))) \
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't be echoed somewhere somehow? Even perhaps in $(WORK_DIR)/python-cc.mk.error.log. Otherwise redirecting it to /dev/null will complexity debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is boring me for a long time, that it looks like an error for packages without python-wheels and regularly no python-cc.mk file.

===>  Processing wheels of {package-name}
cat: /spksrc/spk/{package-name}/work-{arch}-6.1/python-cc.mk: No such file or directory

The only error output that cat might produce is No such file or directory.

The real problem might be that $(WHEELS) is not empty. But anyway this should be moved to a dedicated PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

indeed that is the only side effect but at least we know when it is happening. Why not simply redirect it to stdout (2>&1)?

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.

So really much like the idea of managing at TC_* level the GLIB, GCC and KERNEL which should in theory remove all the UNSUPPORTED and the likes. Proposal's left in my review are:

  • GENERIC_ARCHS vs GO build support - is it handled with your changes?
  • python error redirection to /dev/null
  • renaming x86 to pineview and include it in x64 I guess?

@hgy59
Copy link
Contributor Author

hgy59 commented Dec 22, 2020

@th0ma7 as you introduced the TC_KERNEL variable, can you tell me whether it is mandatory to have the + appended to the version?
As the toolchain files have no + in the name I would like to omit this in TC_KERNEL.

Edit:
@th0ma7 sorry, I didn't await your feedback on this and removed the + in existing TC_KERNEL definitions.

@hgy59
Copy link
Contributor Author

hgy59 commented Dec 22, 2020

Why not rename x86 to pineview so it is consistent?

No, please not.
The official arch name of synology.com ist X86/x86 (look for DS411+ and other atom processors in the arch lists of synology wiki and synocommunity wiki.)

The name pineview appears only in the toolchain file name. And the pineview toolchain is used for the generic x64 arch too.

@th0ma7
Copy link
Contributor

th0ma7 commented Dec 22, 2020

@th0ma7 as you introduced the TC_KERNEL variable, can you tell me whether it is mandatory to have the + appended to the version?
As the toolchain files have no + in the name I would like to omit this in TC_KERNEL.

Edit:
@th0ma7 sorry, I didn't await your feedback on this and removed the + in existing TC_KERNEL definitions.

This will have to be revisited. In order for the modules to properly load it requires a perfect uname match and Synology is adding a + flag at the end of its version such as:

$ uname -r
4.4.59+

I'll check if there is a way to compile the kernel and passing a EXTRAVERSION through a variable instead.

@th0ma7
Copy link
Contributor

th0ma7 commented Dec 22, 2020

I was hoping we would aim towards automating the kernel, gcc and glibc version by pulling it out from the source... But I know that kernel pre-3.something is not easy to reference (e.g. make version). Thnx for going through all the manual work of finding it.

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.

You forgot to modify spksrc.tc.mk in order to echo the new variables:
TC_KERNEL
TC_GCC
TC_GLIBC

@hgy59
Copy link
Contributor Author

hgy59 commented Dec 22, 2020

This will have to be revisited. In order for the modules to properly load it requires a perfect uname match and Synology is adding a + flag at the end of its version such as:

$ uname -r
4.4.59+

I'll check if there is a way to compile the kernel and passing a EXTRAVERSION through a variable instead.

Of all my diskstations only the 4.x kernels have the + with uname -r.

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.

Good catch!

@hgy59
Copy link
Contributor Author

hgy59 commented Dec 23, 2020

@ymartin59 @publicarray please have a look at this PR. As all toolchain makefiles are modified you might have additional ideas for future requirements, particularly related to DSM7.

hgy59 added 10 commits December 29, 2020 01:56
- build hi3535-6.1 (armv7l)
- remove admv7-1.2 from build actions
- x86 (pineview) is for 64-bit atom cpu found in DS411 etc.
- keep i686 as evansport is the only 32-bit intel (atom) cpu so far
- remove LEGACY*_ARCHS definitions
- add variable TC_GCC containing the gcc version of toolchains
- add variable TC_GLIBC containting the glibc version of toolchains
- add missing TC_KERNEL variables to toolchains
- use exact kernel version of toolchain (omit trailing +)
- diskstations models with ppc853x arch are end of life now
- remove ppc853x-5.2 from github build action
@hgy59 hgy59 merged commit 3c934a6 into SynoCommunity:master Dec 29, 2020
@hgy59 hgy59 deleted the update_framework branch December 29, 2020 16:50
hgy59 added a commit to hgy59/spksrc that referenced this pull request Dec 31, 2020
@hgy59 hgy59 mentioned this pull request Dec 31, 2020
hgy59 added a commit that referenced this pull request Dec 31, 2020
hgy59 added a commit to hgy59/spksrc that referenced this pull request Dec 31, 2020
th0ma7 added a commit to th0ma7/spksrc that referenced this pull request Dec 31, 2020
@hgy59 hgy59 mentioned this pull request Dec 31, 2020
hgy59 added a commit that referenced this pull request Dec 31, 2020
@publicarray
Copy link
Member

Just in case it's not obvious: to migrate to the singular toolchain and without redownloading them
just execute mv /spksrc/distrib/toolchains /spksrc/distrib/toolchain in docker.

AlexPresso pushed a commit to AlexPresso/spksrc that referenced this pull request Jan 30, 2025
* framework: redesign arch handling
- extract archs to spksrc.archs.mk
- add x64-4.3 toolchain
- rename AVAILABLE_ARCHS to AVAILABLE_TOOLCHAINS as it defines available arch-tc
- remove GENERIC_ARCHS and ARM7 generic arch as we support generic armv7
- include spksrc.archs.mk instead of spksrc.common.mk for arch definitions
- replace *_ARCHES by *_ARCHS and update all Makefiles for updated ARCHS definitions
- define OLD_PCC_ARCHS and use for unsupported archs where applicable
* adjust and enhance arch specific patch detection
* use 1.2 as legacy version for SRM packages
* add armv7-5.2 toolchain to enable legacy builds
* take default TCVERSION independent of available versions for all-supported target
* fix all-legacy builds
- exclude generic archs from legacy
- remove ARCHS_WITH_KERNEL_SUPPORT as it is the same as LEGACY_ARCHS
* introduce DEPRECATED_ARCHS
* rename spksrc/toolchains to spksrc/toolchain (open issue from SynoCommunity#4143)
* use ARM family names for ARM*_ARCHS
- add hi3535-6.1 (armv7l)  to github build action
- remove admv7-1.2 from build action
- keep i686 as evansport is the only 32-bit intel (atom) cpu so far
* remove non existing toolchain armada37xx-6.2
* add toolchain variables for gcc and glibc versions
- add variable TC_GCC containing the gcc version of toolchains
- add variable TC_GLIBC containting the glibc version of toolchains
- add missing TC_KERNEL variables to toolchains
- use exact kernel version of toolchain (omit trailing +)
* add ppc853x to deprecated archs
- diskstations models with ppc853x arch are end of life now
- remove ppc853x-5.2 from github build action
* cleanup for SynoCommunity#4307
* enable cross/libaom for qoriq
* enable hi3535 builds for golang

Co-authored-by: Vincent Fortier <th0ma7@users.noreply.github.com>
AlexPresso pushed a commit to AlexPresso/spksrc that referenced this pull request Jan 30, 2025
AlexPresso pushed a commit to AlexPresso/spksrc that referenced this pull request Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants