Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

Cloning config and patches from the master branch instead of a specific branch #659

Closed
joshku opened this issue Jul 30, 2019 · 11 comments
Closed
Labels
bug Incorrect behaviour needs-review Needs to be assessed by the team.

Comments

@joshku
Copy link

joshku commented Jul 30, 2019

Description of problem

When trying to build the kernel, I am using the branch stable-1.7 but when running build_kernel.sh, it is cloning from the master branch and not switching to stable-1.7 which results in a missing config file.

[ -d "${patches_path}" ] || git clone "https://${patches_repo}.git" "${patches_repo_dir}"

ERROR: failed to find default config /root/go/src/github.com/kata-containers/packaging/kernel/configs/x86_64_kata_kvm_4.19.x

Looking at the commit history, it appears the config file was dropped in the master branch but the changes haven't made it to the stable-1.7 branch yet.

Expected result

When cloning the repo, it should switch to the branch that is being used and not use the master branch to build

Actual result

It is building the kernel off the master branch and not the branch the user is using.

@joshku joshku added bug Incorrect behaviour needs-review Needs to be assessed by the team. labels Jul 30, 2019
@joshku joshku changed the title Using master branch instead of a specific branch Cloning config and patches from the master branch instead of a specific branch Jul 30, 2019
@grahamwhaley
Copy link
Contributor

/cc @jcvenegas

@ganeshmaharaj
Copy link
Contributor

ganeshmaharaj commented Jul 31, 2019

Guessing we need to backport #646 and #657 to the stable branches.

-- Update. I take the comment back. packaging repo is stable-branchless. The new fragments should be automaticaly taken.

@jcvenegas
Copy link
Member

we recently started to branch packaging, it is a bug if is not able to found the patches directly.
it should work if we do go get pacakging and git checkout stable-1.7 and finally build_kernel.sh .
If in that case it should not need to download anything.

@jcvenegas
Copy link
Member

jcvenegas commented Jul 31, 2019

if packaging is downloaded in a different place (no go get or no GOPATH env var is set ), I agree that the patches and config should checkout to the current branch or something similar.

@joshku
Copy link
Author

joshku commented Jul 31, 2019

When I initially cloned the repo to run build_kernel.sh it is in a different location but within the logs that I see I noticed

INFO: Clone config and patches
Cloning into '/root/go/src/github.com/kata-containers/packaging'...

So it appears to be cloning the repo to another location that is in the GOPATH but fails to realise that it is in a different branch.

@jodh-intel
Copy link
Contributor

It sounds like kernel/build-kernel.sh needs to check to ensure that it is not being run as root. If the users runs ./build-kernel.sh install, the script itself can call sudo for just the calls to install(1).

We need to explain this behavior in the README.md too of course.

@jcvenegas
Copy link
Member

@joshku so to try to summarize the the issue.

  1. clone packaging in a not golang path.
cd /tmp
git clone packaging
cd kernel/

2 ) run the build kernel script

./build-kernel.sh  setup
# This will try to download packaging again because, the packaging repo is not in the gopath.

Is that right ?

But works if instead you do, something like:

go get github.com/kata-containers/packaging 
git checkout stable-1.7
cd  $GOPATH/src/github.com/kata-containers/packaging/kernel
./build-kernel.sh  setup
...

@joshku
Copy link
Author

joshku commented Aug 1, 2019

So the first and second step sound correct. What I did to get it to work is just to adjust the code at line 161 of build_kernel.sh to add the -b option. So instead of just a git clone <repo_url> it is now git clone -b <branch_name> <repo_url> and then it worked.

I suspect what you are saying is correct. I am not really sure what patches_repo_dir points to. But not really a 100% confident.

@joshku
Copy link
Author

joshku commented Aug 1, 2019

I believe that regardless of what I did, within the build_kernel.sh script it still calls a git clone of the packaging repo but it doesn't know if you are executing the script from a different branch than master. Therefore, when it is grabbing the kernel config and patches, it is grabbing it from the freshly cloned directory regardless of whether or not I have already downloaded the repo or not.

@jcvenegas
Copy link
Member

jcvenegas commented Aug 1, 2019

Rigth, so when I did those scripts I was expecting that people do a go get, and the extra git clone is if for some reason just download the script so it could get them. But seems really confusing flow.

So we can modify the script
to
https://github.com/kata-containers/packaging/blob/master/kernel/build-kernel.sh#L33 use the script dir

-readonly patches_repo_dir="${GOPATH}/src/${patches_repo}"
# Default path to search patches to apply to kernel
-readonly default_patches_dir="${patches_repo_dir}/kernel/patches/"
+readonly default_patches_dir="${script_dir}/patches/"
# Default path to search config for kata
-readonly default_kernel_config_dir="${GOPATH}/src/${kernel_config_repo}/kernel/configs"
+readonly default_kernel_config_dir="${script_dir}/configs"
# Default path to search for kernel config fragments
-readonly default_config_frags_dir="${GOPATH}/src/${kernel_config_repo}/kernel/configs/fragments"
+readonly default_config_frags_dir="${script_dir}/configs/fragments"

And instead of download again the repo, just fail.

https://github.com/kata-containers/packaging/blob/master/kernel/build-kernel.sh#L238

-[ -d "${patches_path}" ] || git clone "https://${patches_repo}.git" "${patches_repo_dir}"
+[ -d "${patches_path}" ] || die "patches path ${patches_path} not found" # or a warning only.

@joshku
Copy link
Author

joshku commented Aug 2, 2019

ahh I see. That makes a lot more sense. I like this flow because if we keep the git clone part we would have to ensure it clones and checkouts the correct branch to be used which is a bigger hassle.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Incorrect behaviour needs-review Needs to be assessed by the team.
Projects
None yet
Development

No branches or pull requests

5 participants