Skip to content
This repository has been archived by the owner on Jan 6, 2023. It is now read-only.

ostro-os: update to latest HEAD #20

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

avalluri
Copy link
Contributor

@avalluri avalluri commented Sep 8, 2016

test-build: Update ostro-os to latest HEAD (as per @mythi request)

Signed-off-by: Amarnath Valluri [email protected]

@@ -0,0 +1,3 @@
SRC_URI[md5sum] = "075996be339ab16ad7b94d6de3ee07bd"
SRC_URI[sha256sum] = "77b8a7fd9393d10def665658a41176ee745d5c7969a4a0f43cefcc8a4cd90947"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still wrong? IIRC 1.5-1 should fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mythi Yes, bitbake complained abt the mismatch.

ERROR: checksec-1.5-1-r0 do_fetch: Checksum failure fetching http://www.trapkit.de/tools/checksec.sh
ERROR: checksec-1.5-1-r0 do_fetch: Function failed: Fetcher failure for URL: 'http://www.trapkit.de/tools/checksec.sh'. Checksum mismatch!
File: '/var/tmp/ostro-os-xt/bb-cache/sources/checksec.sh' has md5 checksum 075996be339ab16ad7b94d6de3ee07bd when 57cc3fbbbe48e8ebd4672c569954374d was expected
File: '/var/tmp/ostro-os-xt/bb-cache/sources/checksec.sh' has sha256 checksum 77b8a7fd9393d10def665658a41176ee745d5c7969a4a0f43cefcc8a4cd90947 when 05822cd8668589038d20650faa0e56f740911d8ad06f7005b3d12a5c76591b90 was expected
If this change is expected (e.g. you have upgraded to a new version without updating the checksums) then you can use these lines within the recipe:
SRC_URI[md5sum] = "075996be339ab16ad7b94d6de3ee07bd"
SRC_URI[sha256sum] = "77b8a7fd9393d10def665658a41176ee745d5c7969a4a0f43cefcc8a4cd90947"

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case the .sh comes from the source mirror it seems. We should probably get rid of that mirrored copy and fix ostro-os-xt master with: ostroproject/ostro-os@913b474

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mythi How can we get rid of the mirror?, you mean, should we touch the recipe version?

Copy link
Contributor

Choose a reason for hiding this comment

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

@avalluri yes we need to take checksec_1.5-1.bb in ostro-os-xt. (Then in this PR you revert the commit (because the same .bb comes from ostro-os).)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mythi, With this PR ostro-os-xt is using checksec_1.5-1.bb which is coming from ostro-os only.

I tried re-build checksec without this patch by disabling source mirror(setting OSTRO_SOURCE_MIRROR_URL in local.conf), this way force fetched from the upstream, which resolved the mismatch issue. Hence reverting my commit.

But i am not sure, how it will be resolved at CI environment.

Copy link
Contributor

Choose a reason for hiding this comment

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

@avalluri we need to get checksec_1.5-1.bb in ostro-os-xt because everybody building the distro without OSTRO_SOURCE_MIRROR_URL gets the failure. The old copy of checksec.sh only exists in our mirror.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mythi Anyway I am patching the checkseck recipe to avoid this mirroring issue, hopefully should fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mythi In your opinion does this makes any sense:
avalluri/meta-security-isafw@cd0cc55

ipuustin pushed a commit to ipuustin/ostro-os-xt that referenced this pull request Sep 16, 2016
installer: fix booting from MicroSD
@mythi
Copy link
Contributor

mythi commented Sep 19, 2016

On Mon, Sep 19, 2016 at 2:47 PM, Amarnath Valluri [email protected]
wrote:

@avalluri commented on this pull request.

In meta-ostro-xt/recipes-devtools/checksec/checksec_1.5-1.bbappend
#20:

@@ -0,0 +1,3 @@
+SRC_URI[md5sum] = "075996be339ab16ad7b94d6de3ee07bd"
+SRC_URI[sha256sum] = "77b8a7fd9393d10def665658a41176ee745d5c7969a4a0f43cefcc8a4cd90947"

@mythi https://github.com/mythi In your opinion does this makes any
sense:
avalluri/meta-security-isafw@cd0cc55
avalluri/meta-security-isafw@cd0cc55

It looks fine but we'd anyway need to fix the build for ostro-os-xt. With
the fix avalluri/meta-security-isafw@cd0cc55
would not be needed.

@avalluri avalluri force-pushed the test-build branch 9 times, most recently from 59197aa to b2ea48a Compare September 24, 2016 05:04
Copy link
Contributor

@kad kad left a comment

Choose a reason for hiding this comment

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

Fix errors in new patches. Don't try to hide them.

# hence we need to setup the git configuration
git config --global user.name "$GIT_COMMITTER_NAME"
git config --global user.email "$GIT_COMMITTER_EMAIL"

# FIXME: undbound variables used without checking:
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't do that. Patches must have origin information. This will hide that error situation.

Copy link
Contributor Author

@avalluri avalluri Sep 26, 2016

Choose a reason for hiding this comment

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

@kad, This is not just related to git apply or patch formatting, As i mentioned in commit message the real issue is with the git environment in bitbake tasks, though we set GIT_COMITTER_{NAME/EMAIL}, those are not available while patching source. So here the idea is to pass them via 'git config'

Copy link
Contributor

Choose a reason for hiding this comment

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

If patch in your changeset would have proper information in header, those wouldn't be needed.

Copy link
Contributor Author

@avalluri avalluri Oct 6, 2016

Choose a reason for hiding this comment

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

@kad, This issue raised on yocto project by @mythi and they accepted to fix in oe-core. Hopefully we need not to set this configuration as oecore itself provide dummy git config.

https://bugzilla.yoctoproject.org/show_bug.cgi?id=10346

We can revert this once the fix is available.

Copy link
Contributor

Choose a reason for hiding this comment

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

let's get fix from oe-core (Paul's change to patch functions). it's better than trying to create local workarounds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Paul's fix your are referring(http://git.yoctoproject.org/cgit/cgit.cgi/poky/commit/?id=4b4387455c62cf19fa3d215a11b5d0b1211d4570) does not fix kernel patching issue, that address only PATCHTOOL='git' case.

This is the fix we needed:
http://lists.openembedded.org/pipermail/openembedded-core/2016-October/127230.html

Copy link
Contributor

Choose a reason for hiding this comment

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

in reality, I think both of those patches are needed. we stepped on one case and issue present in different scenarios.

@@ -11,9 +11,12 @@ BUILD_CACHE_DIR=$WORKSPACE/bb-cache
BUILDOS="opensuse-42.1"
GIT_PROXY_COMMAND=oe-git-proxy
TARGET_MACHINE="intel-corei7-64"
GIT_COMMITTER_EMAIL=${GIT_COMMITTER_EMAIL:-$(git config --get user.email)}
GIT_COMMITTER_NAME=${GIT_COMMITTER_NAME:-$(git config --get user.name)}
GIT_COMMITTER_NAME=${GIT_COMMITTER_NAME:-${USER}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't do that. Git apply error is real error. Fix it, instead of trying to hide.


BUILD_ARGS="--build-arg uid=`id -u`"
RUN_ARGS="-u `id -u`"
RUN_ARGS=(-u "`id -u`")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is that change ?

@@ -22,7 +25,7 @@ for proxy in $safe_proxy_vars; do
# strip spaces from values, if any.
val="`echo ${!proxy} | tr -d ' '`"
BUILD_ARGS="$BUILD_ARGS --build-arg $proxy=${val}"
RUN_ARGS="$RUN_ARGS -e $proxy=${!proxy}"
RUN_ARGS+=(-e "$proxy=${!proxy}")
Copy link
Contributor

Choose a reason for hiding this comment

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

why is that here ?


docker build -t $CURRENT_PROJECT $BUILD_ARGS $WORKSPACE/docker/$BUILDOS

if [ ! -d $BUILD_CACHE_DIR ]; then
mkdir -p $BUILD_CACHE_DIR
fi

docker run -it --rm $RUN_ARGS \
docker run -it --rm "${RUN_ARGS[@]}" \
Copy link
Contributor

Choose a reason for hiding this comment

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

why to have this bashism ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kad, Current script fails to run, if any of the docker run environment we pass has white spaces in it. I tried to solve this by using bash arrays.

Copy link
Contributor

Choose a reason for hiding this comment

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

any space in this subset of variables usually means syntax error, which would be visible if not in this script, but then further down the line. Better sanitize values near line 25.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may not be the case for GIT_COMITTER_NAME(though its still open :)), where space is valid. That is what actually made visible this issue.
The other possibility where space is valid is, passing {build_target(s)} to the script via command line arguments, to build custom targets(this is how i am using this script with my local changes). This way we can use all bitbake options with this script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kad As missing git config issue is been fixed in oe-core itself, I feel we can live without this change. The main motivation behind this change was passing GIT_COMITTER_NAME to docker, where space is valid.

Copy link
Contributor

Choose a reason for hiding this comment

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

GIT_* variables I don't think it's really good idea to pass. Same with *_proxy variables: better to sanitize content of *_proxy variables.
However, if you split patch for making RUN_ARGS as array and test it under different versions of bash (3 and 4 at least), I think it can be accepted.
Another potential enhancement that can be done in build scripts is to parse BB_ENV_EXTRAWHITE and pass those as arguments to docker calls. that way user would have explicit way to pass something from his development environment into isolated container.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kad, I couldn't understand fully, about splitting this patch. This patch is touching only RUN_ARGS.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, put only this patch that touches RUN_ARGS into separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kad, I put this commit in PR #44
As you suggested, I tested this change both on bash v4.2 & v3.2. But our current script does not work with v3.2 because 'test/if' in bash-v3 does not support '-v' option, other than that, this change work on both versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm. interesting. that's something that also might need to be fixed. I'll check. Let's continue discussion about that part in #44

@@ -0,0 +1,25 @@
diff --git a/configure.ac b/configure.ac
Copy link
Contributor

Choose a reason for hiding this comment

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

This patch must have proper origin header, to clearly specify where it comes from, who is author, upstream status, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kad, Thanks for pointing out, I was in impression that git formatted patch fail to apply on non-git source, now I git formatted the patch.

Copy link
Contributor

Choose a reason for hiding this comment

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

@avalluri what were the other patches git apply failed and f089278 was needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mythi, Currently there are two patches in kernel patch series on XT:

  1. librealsense_formats_linux-yocto_4.4.patch
  2. 0001-ovl-setxattr-don-t-deadlock-when-called-from-ima_fix.patch

Both fail to apply if we not provide committer details either via environment or via global git config.
This is not visible on non-docker builds as, usually host .gitconfig set properly, But not the case with docker container.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks

  1. we have reported to meta-intel-realsense. 2. is ours but it looks correct. Any idea why it fails?

Copy link
Contributor Author

@avalluri avalluri Sep 26, 2016

Choose a reason for hiding this comment

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

Almost all git actions(atleast in this context am,apply) look for committer details before applying the patch. That's what i observed with this exercise :)

Copy link
Contributor

Choose a reason for hiding this comment

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

that's the reason for clean build inside container: catch inconsistencies in data and patches :)

@avalluri avalluri force-pushed the test-build branch 3 times, most recently from 0f3154a to c8f7297 Compare September 30, 2016 10:35
@avalluri avalluri force-pushed the test-build branch 5 times, most recently from 043e380 to 2d29de2 Compare October 6, 2016 06:46
@avalluri
Copy link
Contributor Author

avalluri commented Oct 6, 2016

@kad As you suggested made a new PR for build script change: #44

@avalluri avalluri force-pushed the test-build branch 2 times, most recently from f656c18 to 79026c7 Compare October 7, 2016 12:23
@avalluri
Copy link
Contributor Author

avalluri commented Oct 7, 2016

@kad Now i reverted the change(setting git config inside the docker) you are not happy with :) can you please review if the new change (79026c7) is ok.

@avalluri avalluri force-pushed the test-build branch 2 times, most recently from f38ab2d to 665ed11 Compare October 14, 2016 11:59
@avalluri avalluri force-pushed the test-build branch 3 times, most recently from f43ef78 to 1bdd41f Compare October 20, 2016 12:39
Signed-off-by: Amarnath Valluri <[email protected]>
In order to allow meta-oe layer updates, rename upower bbappend with more
generic version.

Signed-off-by: Amarnath Valluri <[email protected]>
Patched source to respect LDFLAGS.

Patch submitted to meta-intel-relasense layer:
IntelRealSense/meta-intel-realsense#4

Signed-off-by: Amarnath Valluri <[email protected]>
When build with gcc5, build was failing with errors:
| ../src/image-reader.cpp: In member function 'bool PNGReader::init(const
string&)':
| ../src/image-reader.cpp:123:16: error: 'unique_ptr' in namespace 'std' does
not name a template type
|      const std::unique_ptr<std::istream>
is_ptr(Util::get_resource(filename));
|                 ^
| ../src/image-reader.cpp:124:12: error: 'is_ptr' was not declared in this
scope
|      if (!(*is_ptr)) {
|             ^
| ../src/image-reader.cpp:149:57: error: 'is_ptr' was not declared in this
scope
|      png_set_read_fn(priv_->png, reinterpret_cast<void*>(is_ptr.get()),
|                                                          ^
| ../src/model.cpp: In member function 'bool Model::load_3ds(const string&)':
| ../src/model.cpp:364:16: error: 'unique_ptr' in namespace 'std' does not name
a template type
|      const std::unique_ptr<std::istream>
input_file_ptr(Util::get_resource(filename));
|                 ^
| ../src/model.cpp:365:31: error: 'input_file_ptr' was not declared in this
scope
|      std::istream& input_file(*input_file_ptr);
|                                ^
| ../src/model.cpp: In member function 'bool Model::load_obj(const string&)':
| ../src/model.cpp:684:16: error: 'unique_ptr' in namespace 'std' does not name
a template type
|      const std::unique_ptr<std::istream>
input_file_ptr(Util::get_resource(filename));
|                 ^
| ../src/model.cpp:685:30: error: 'input_file_ptr' was not declared in this
scope
|      std::istream& inputFile(*input_file_ptr);

Signed-off-by: Amarnath Valluri <[email protected]>
Patched Makefile to make use of LDFLAGS

Signed-off-by: Amarnath Valluri <[email protected]>
As latest(v1.8.0) gstreamer-vaapi is having hard dependency on gstreamer-codecs
which are part of bad plugins, we are falling back to old version.

This is a temporary fix.

Signed-off-by: Amarnath Valluri <[email protected]>
Randomly build fails with below error:

   Generating local configuration database from kernel ...make[1]: ***
   No rule to make target 'kernelversion'.  Stop.
   Kernel version parse failed!
   make: *** [Makefile:42: defconfig-iwlwifi-public] Error 1

The reason for this failure is 'make defconfig-iwlwifi-public' in do_configure
task trys to get 'kernelverison' from kernel scripts. Kernel scripts are
generated by 'do_make_scripts'. The do_make_script is run before running
do_compile. So at the time of do_configure, kernel sripts are not ready,
hence 'make kernelversion' fails.

This change moves 'make defconfig-iwlwifi-public' to do_comiple.

Signed-off-by: Amarnath Valluri <[email protected]>
Manpage generation using(a2x) xmllint, which needs network to download custom xml DTD.
Fails with below error:

| a2x: executing: "xmllint" --nonet --noout --valid "/home/avalluri/ostro-os-xt/build/tmp-glibc/work/corei7-64-ostro-linux/ocl-icd/2.2.9-r0/ocl-icd-2.2.9/doc/libOpenCL.7.xml"
|
| I/O error : Attempt to load network entity http://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd
| /home/avalluri/ostro-os-xt/build/tmp-glibc/work/corei7-64-ostro-linux/ocl-icd/2.2.9-r0/ocl-icd-2.2.9/doc/libOpenCL.7.xml:2: warning: failed to load external entity "http://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd"
| D DocBook XML V4.5//EN" "http://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd"
|                                                                                ^

Patched configure.ac to add config option to enable/disable document generation.

Signed-off-by: Amarnath Valluri <[email protected]>
Commit 920baaa6eb4c5094b7fe36736c598badf43350dc on oe-core added 'libsrvg-gtk'
dependency to packagegroup-xfce-base.bb, to support viewing of svg images.

Signed-off-by: Amarnath Valluri <[email protected]>
The changes of this bbappend have been applied on oe-core. Hence this change is
not needed anymore.

Signed-off-by: Amarnath Valluri <[email protected]>
Due to lack of task dependency between 'do_removebinaries' and 'do_removecruft'
when both run parallelly sometimes, 'find' in do_removebinaries referring on directories
that sare already removed by do_removecruft.

This change makes sure that both tasks execute sequentially.

Signed-off-by: Amarnath Valluri <[email protected]>
This issue been addressed in oecore here:
http://lists.openembedded.org/pipermail/openembedded-core/2016-October/127230.html

Till we take 25b43cb05c645e43f96bc18906441b8fdc272228 from oecore, this is a
temporary fix to whitelist and pass git committer environment to do_patch task.

Signed-off-by: Amarnath Valluri <[email protected]>
@okartau
Copy link
Contributor

okartau commented Oct 24, 2016

test this please

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants