crash 8.0.6 -> 9.0.2 + crash-memory-driver#1056
Conversation
|
I think its using https://autobuilder.yoctoproject.org/valkyrie/api/v2/logs/7106116/raw_inline |
OldManYellsAtCloud
left a comment
There was a problem hiding this comment.
I have added a couple notes and questions, mostly small nitpicks.
| -# This program is distributed in the hope that it will be useful, | ||
| -# but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| -# GNU General Public License for more details. |
There was a problem hiding this comment.
Can we just leave the license part as it is?
There was a problem hiding this comment.
Yes, we can keep the license of the Makefile: 7998a78
There was a problem hiding this comment.
Looking at the referenced commit (crash-utility/crash@772fbb1), it seems to be included starting version 9.0.0 - though it wasn't included in the new patch for gdb 16.2.
Is this patch still needed?
I got the same question for 0005-Fix-build-failure-on-32bit-machine-i686.patch file.
There was a problem hiding this comment.
To be honest, I have no idea. I kept the patches, which were cleanly applied.
There was a problem hiding this comment.
Looking at it closer, both of these are dangling patches, neither of them are used in the recipes. They can be dropped.
| HOMEPAGE = "https://github.com/crash-utility/crash/tree/master/memory_driver" | ||
| RECIPE_MAINTAINER = "Robert Berger <Robert.Berger@ReliableEmbeddedSystems.com>" | ||
| LICENSE = "GPL-2.0-only" | ||
| LIC_FILES_CHKSUM = "file://${COMMON_LICENSE_DIR}/GPL-2.0-only;md5=801f80980d171dd6425610833a22dbe6" |
There was a problem hiding this comment.
Not a very strong opinion, but I think referencing the source file's license declaration would be better: https://github.com/crash-utility/crash/blob/master/memory_driver/crash.c#L9
Also, it seems to be gpl-2-or-later, not only
There was a problem hiding this comment.
I can reference the source file's license declaration. It looks like gpl-2-or-later, but the last line says MODULE_LICENSE("GPL"); which means it's GPL-2.0-only. This is needed if a kernel module needs to have access to all kernel symbols.
There was a problem hiding this comment.
Haha, yeah, ambiguous license is always great.
Personally I consider the license's text itself to be stronger than the macro (I believe the kernel also only cares about being compatible with gpl2, though I might have dreamed that up).
I won't die on this hill though.
There was a problem hiding this comment.
note to myself: done locally
| file://0001-hacked-Makefilefor-module.bbclass.patch;striplevel=2 \ | ||
| file://0002-add-sparse-support.patch;striplevel=2 \ | ||
| " | ||
| PV = "9.0.2+git${SRCPV}" |
There was a problem hiding this comment.
SRCPV is not needed anymore, it can be dropped
There was a problem hiding this comment.
note to myself: done locally
| SUMMARY = "Kernel analysis utility for live systems and core dumpfiles" | ||
| DESCRIPTION = "The core analysis suite is a self-contained tool that can be used to \ | ||
| investigate either live systems, kernel core dumps created from kdump, or mcore dumpfiles." | ||
| HOMEPAGE = "https://github.com" |
There was a problem hiding this comment.
Probably https://github.com/crash-utility/crash/ or https://crash-utility.github.io/ would be better for homepage
There was a problem hiding this comment.
note to myself: done locally
| export LDFLAGS | ||
| export AR | ||
| export LD | ||
| export RANLIB |
There was a problem hiding this comment.
Are these exports needed explicitly? Most (all?) of these are exported by default. Or am I missing some context?
There was a problem hiding this comment.
note to myself: done locally (removed all the exports not just line 73 to 76. My test builds went through without the exports.)
| # build_target: X86_64 | ||
| # build_version: 9.0.2 | ||
| # compiler version: gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0 | ||
| # <-- |
There was a problem hiding this comment.
Was this comment left here accidentally? Or is it useful for something?
There was a problem hiding this comment.
It proves that 0001-Use-CC-env-var-to-get-compiler-version.patch is applied and so the right compiler is being picked up. But I can remove the comments.
note to myself: done locally
| # http://errors.yoctoproject.org/Errors/Details/186964/ | ||
| COMPATIBLE_HOST:libc-musl = 'null' | ||
| ERROR_QA:remove = "buildpaths" | ||
| WARN_QA:append = " buildpaths" |
There was a problem hiding this comment.
If this is really unavoidable, could you please add some notes about it in the commit message (or even as a comment in the recipe), why this is needed?
There was a problem hiding this comment.
I added the following:
# WARNING: crash-9.0.2-r0 do_package_qa: QA Issue: File /usr/bin/crash in package crash contains reference to TMPDIR [buildpaths]
#
# WARNING: crash-cross-canadian-aarch64-9.0.2-r0 do_package_qa: QA Issue: File /opt/phytec-ampliphy-resy-systemd/6.0.98-devel/sysroots/x86_64-resysdk-linux/usr/bin/aarch64-resy-linux/crash in package crash-cross-canadian-aarch64 contains reference to TMPDIR [buildpaths]
#
ERROR_QA:remove = "buildpaths"
WARN_QA:append = " buildpaths"
note to myself: done locally
There was a problem hiding this comment.
I mean more like, why does this error happen now? Can it be corrected, instead of being ignored? It happens with some recipes that there is no choice but to ignore the QA error, but in case of this recipe this is a regression, it wasn't needed so far. The real question is, what happened that made this needed?
As a sidenote, I suspect the --with-sysroot=${STAGING_DIR_TARGET} is causing this in EXTRA_OEMAKE. Should this rather be --with-build-sysroot=${STAGING_DIR_TARGET}? (This is mostly a blind guess after taking a quick glance at the config file and the error. It can be totally wrong.)
| ${GNU_MIRROR}/gdb/gdb-16.2.tar.gz;name=gdb;subdir=gdb \ | ||
| file://7003cross_ranlib.patch \ | ||
| file://0001-cross_add_configure_option.patch \ | ||
| file://donnot-extract-gdb-during-do-compile.patch \ |
There was a problem hiding this comment.
Not all of these patch files were removed from the tree (e.g. this donnot...patch) - could you please also drop all the files that are not needed anymore?
There was a problem hiding this comment.
I am not sure if they are needed or not. We will see once people start using the new crash version. That's the reason I left them there. But sure, I can remove them for now and keep only the ones that I use.
note to myself: done locally
|
|
||
| # 5. Return to the active workspace build root to merge downstream additions | ||
| cd ${B} | ||
| if [ -f "gdb-16.2.patch" ]; then |
There was a problem hiding this comment.
I think this is overly defensive, and this if could be dropped: If upstream renames this patch after an update, it is easy to miss, because the lack of patch is swallowed silently. I think we should complain loud if this file doesn't exist, so the recipe can be updated also.
There was a problem hiding this comment.
note to myself: done locally
|
@RobertBerger please squash all changes into one commit. |
Security fixes: ================= - Remove import-time loading of timezone offset data from pickle to prevent unsafe deserialization from packaged data - Replace eval() use when parsing no_word_spacing with strict boolean parsing to prevent code execution from locale metadata (openembedded#1056) New features: ============= - Add support for expressions like "N {interval} from now" in English (#1271) - Add support for the en-US locale (#1222) Fixes: ======== - Honor REQUIRE_PARTS for ambiguous month-number inputs by retrying with a year-biased DATE_ORDER (#1298) - Fix parsing word-number relative phrases such as "two days later" (#1316) - Allow md5hash to work in FIPS environments (#1267) Improvements: ============= - Add Bosnian Cyrillic (ijekavica) date translations (#1293) - Add a new browser-based demo to the project documentation (#1306) - Update installation documentation to replace setup.py install guidance - Add a project security policy Signed-off-by: Wang Mingyu <wangmy@fujitsu.com> Signed-off-by: Khem Raj <khem.raj@oss.qualcomm.com>
1) crash-memory-driver: fixed typo to include proper patch
2) crash-memory-driver: keep license,...
patch against comments on pull request:
1) HOMEPAGE updated
2) UPSTREAM_CHECK_URI updated
3) exports removed
4) if [ -f "gdb-16.2.patch" ]; then... removed
5) notes about QA Issue: reference to TMPDIR [buildpaths] added
6) notes ./crash --buildinfo removed
7) LIC_FILES_CHKSUM: use license text in crash.c
8) PV = "9.0.2+git${SRCPV}" -> SRCPV removed
9) removed unused patches
10) crash: LICENSE = GPL-3.0-only
80c07da to
089c28d
Compare
I tried to squash it all into a single commit. Please check. |
Updated crash recipe from 8.0.6 to 9.0.2
crash-nativebuilds and runscrash-cross-canadian-aarch64builds—I did not build an SDK to see if it runs.crashfor aarch64 target builds and runsAdded recipe for crash memory driver