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

Add package builds #1965

Merged
merged 12 commits into from
May 12, 2018

Conversation

ingwinlu
Copy link
Contributor

@ingwinlu ingwinlu commented May 8, 2018

Purpose

Enable debian package generation on the build server to replace the existing one.

Checklist

  • commit messages are fine ("module: short statement" syntax and refer to issues)
  • affected documentation is fixed
  • release notes are updated (doc/news/_preparation_next_release.md)

resolves #1911
resolves #1752

@ingwinlu
Copy link
Contributor Author

ingwinlu commented May 9, 2018

jenkins build jenkinsfile[package/stretch] please

@ingwinlu
Copy link
Contributor Author

ingwinlu commented May 9, 2018

@markus2330 after struggling with jenkins way of handling git checkouts (so much automagic going on) I got the build going. However the test failures do not appear when I run it locally so I will have to look into it.

@ingwinlu
Copy link
Contributor Author

ingwinlu commented May 9, 2018

jenkins build jenkinsfile please

@ingwinlu
Copy link
Contributor Author

ingwinlu commented May 9, 2018

@petermax2 any idea why https://github.com/ElektraInitiative/libelektra/blob/master/src/plugins/crypto/test_internals.h#L263 might throw an error here? When I run the command locally (in the same docker image) it passes...

@petermax2
Copy link
Member

any idea why https://github.com/ElektraInitiative/libelektra/blob/master/src/plugins/crypto/test_internals.h#L263 might throw an error here? When I run the command locally (in the same docker image) it passes...

Maybe the GPG key is already installed on the build server, so that GPG does not exit with 0? But this is just a wild guess.

@ingwinlu
Copy link
Contributor Author

ingwinlu commented May 9, 2018

jenkins build jenkinsfile[package/stretch] please

@ingwinlu
Copy link
Contributor Author

ingwinlu commented May 9, 2018

Maybe the GPG key is already installed on the build server, so that GPG does not exit with 0? But this is just a wild guess.

Should not be the case as the docker container should be 'fresh' on the build server as well.

It seems like errorKey is empty. Or am I using elektra wrong?

@petermax2
Copy link
Member

What content does msg have? Maybe the stderr output of GPG is empty, but there might be a response in stdout (which goes to msg if I am not mistaking).

@ingwinlu
Copy link
Contributor Author

ingwinlu commented May 9, 2018

@petermax2 msg only holds the private key sent to gpg

@petermax2
Copy link
Member

msg only holds the private key sent to gpg

Hm, no errorMsg and no msg. That's weird.

@ingwinlu
Copy link
Contributor Author

ingwinlu commented May 9, 2018

Since it is a docker only problem I assume it is related to tty issues. error code returned is 512 but I can not get it to print an error message

@ingwinlu
Copy link
Contributor Author

ingwinlu commented May 9, 2018

jenkins build jenkinsfile[package/stretch] please

@ingwinlu
Copy link
Contributor Author

ingwinlu commented May 9, 2018

@markus2330 any insight on why testmod_blockresolver is failing? maybe we can apply that to the crypto stuff

@markus2330
Copy link
Contributor

No, I can only guess: The genTempFilename could be non-unique: It uses tv.tv_usec but maybe docker does not give precise time? @tom-wa do you have any insight?

My idea was that both fcrypt and blockresolver work with temp files. But it easily could be a wrong track.

@markus2330
Copy link
Contributor

I now get similar errors when building the package:

 42/118 Test  #48: testmod_blockresolver ............***Failed    0.00 sec
BLOCKRESOLVER     TESTS
==================

/home/markus/Projekte/Elektra/libelektra/tmpbuilddir/libelektra/src/plugins/blockresolver/testmod_blockresolver.c:70: error in test_BlockresolverWrite: blockresolver->kdbSet failed
Compare <key = only the inside has changed
>, with <key = inside block
>
in file /home/markus/Projekte/Elektra/libelektra/tmpbuilddir/libelektra/src/plugins/blockresolver/blockresolver/compare.block, line 5.
/home/markus/Projekte/Elektra/libelektra/tmpbuilddir/libelektra/tests/cframework/tests.c:157: error in compare_line_files: comparing lines failed
/home/markus/Projekte/Elektra/libelektra/tmpbuilddir/libelektra/src/plugins/blockresolver/testmod_blockresolver.c:72: error in test_BlockresolverWrite: files do not match as expected

testmod_blockresolver Results: 15 Tests done — 3 errors
        Start  73: testmod_fcrypt
 57/118 Test  #73: testmod_fcrypt ...................***Failed    0.01 sec
FCRYPT       TESTS
==================

/home/markus/Projekte/Elektra/libelektra/tmpbuilddir/libelektra/src/plugins/fcrypt/testmod_fcrypt.c:187: error in test_file_crypto_operations: kdb set failed
/home/markus/Projekte/Elektra/libelektra/tmpbuilddir/libelektra/src/plugins/fcrypt/testmod_fcrypt.c:188: error in test_file_crypto_operations: file content did not change during encryption
/home/markus/Projekte/Elektra/libelektra/tmpbuilddir/libelektra/src/plugins/fcrypt/testmod_fcrypt.c:191: error in test_file_crypto_operations: kdb get (pregetstorage) failed
/home/markus/Projekte/Elektra/libelektra/tmpbuilddir/libelektra/src/plugins/fcrypt/testmod_fcrypt.c:195: error in test_file_crypto_operations: kdb get (postgetstorage) failed
/home/markus/Projekte/Elektra/libelektra/tmpbuilddir/libelektra/src/plugins/fcrypt/testmod_fcrypt.c:196: error in test_file_crypto_operations: postgetstorage did not encrypt the file again
/home/markus/Projekte/Elektra/libelektra/tmpbuilddir/libelektra/src/plugins/fcrypt/testmod_fcrypt.c:231: error in test_file_signature_operations: kdb set failed
/home/markus/Projekte/Elektra/libelektra/tmpbuilddir/libelektra/src/plugins/fcrypt/testmod_fcrypt.c:232: error in test_file_signature_operations: file content did not change during encryption
/home/markus/Projekte/Elektra/libelektra/tmpbuilddir/libelektra/src/plugins/fcrypt/testmod_fcrypt.c:235: error in test_file_signature_operations: kdb get failed

So at least these two errors are not related to docker.

@markus2330
Copy link
Contributor

markus2330 commented May 10, 2018

Btw. if building the packages fails because of some python files (not installed), please revert b0e54fc

@ingwinlu
Copy link
Contributor Author

ingwinlu commented May 10, 2018

But it works locally for me, something is effed up bad.
I will rebuild my docker image locally with the latest debian stretch image (mine was a few weeks old). maybe that was it (that would also explain it failing outside of docker if it is debian release related).

@markus2330
Copy link
Contributor

markus2330 commented May 10, 2018

Weird, I get these two errors reproducible on two Debian stretch machines, regardless of if I cleaned up /tmp files.

@ingwinlu
Copy link
Contributor Author

still can not reproduce it and I have run out of ideas on what I could try

@ingwinlu
Copy link
Contributor Author

@markus2330 can you run a printenv | sort on the machines where you could reproduce it? I can not even get it to reproduce with the same docker image on a7...

@markus2330
Copy link
Contributor

env -i PATH=$PATH scripts/build-debian-package and env -i PATH=/usr/bin:/bin scripts/build-debian-package also fails with the same two test cases, so the env is irrelevant.

I am afraid we will have to bisect which change introduced the problem. Seems like the build job was long enough not working to let new errors creep in.

Thanks for your work on the build server. There is evidence again and again that it is really important to detect faulty commits quickly. In the situation we are now, it is really hard to find out why this fails.

@ingwinlu
Copy link
Contributor Author

strangely enough the build on stretch mr still passes the tests and only fails when signing so I don't think there is a faulty commit that we can pinpoint the behavior to. More likely something in the environment is inconsistent that would explain why it works on some machines and some not.

however i have no idea why i can not reproduce it with the docker container when running the steps independently...

@ingwinlu
Copy link
Contributor Author

jenkins build jenkinsfile please

@ingwinlu ingwinlu requested a review from markus2330 May 12, 2018 16:01
@markus2330
Copy link
Contributor

Thank you for your great last-minute-commitment to finally fix the stretch repos and the nice release notes!

A small remark: The convention is to set "ready to merge" not before the build server finished.

Copy link
Contributor

@markus2330 markus2330 left a comment

Choose a reason for hiding this comment

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

Thank you, looks great. As always, more docu welcomed!

responsivness.
It focuses heavily on containerisation to improve hardware utilization.
If you are interested in `#devops` have a look at our
[Jenkinsfile](https://github.com/ElektraInitiative/libelektra/blob/master/scripts/jenkins/Jenkinsfile).
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you write a line or two about how to get docker images locally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently the setup does not allow unauthenticated pulls from our repository.

Add it to your `sources.list`:
```
deb [trusted=yes] https://debian-stretch-repo.libelektra.org/ stretch main
deb-src [trusted=yes] https://debian-stretch-repo.libelektra.org/ stretch main
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also add to doc/INSTALL.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already did.

withEnv(["DEBSIGN_PROGRAM=gpg",
"DEBFULLNAME=Jenkins (User for Elektra automated build system)",
"DEBEMAIL=autobuilder@libelektra.org"]) {
sh "rm -R ./*"
Copy link
Contributor

Choose a reason for hiding this comment

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

sounds dangerous. Maybe put into a function "initialCleanup"?

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 isn't, it is guaranteed to be in a jenkins workspace in a 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.

A function seems to be useful nevertheless, even though it is trivial.

Btw. Why not delete files starting with .?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know I used it in the very same file. But it behaves differently as it deletes the current directory, not just the content of the current directory.

return tasks
}

def build_package_debian_stretch() {
Copy link
Contributor

Choose a reason for hiding this comment

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

why are some functions camel case and others use _ ? (Not to be changed in this PR, though)

Copy link
Contributor Author

@ingwinlu ingwinlu May 12, 2018

Choose a reason for hiding this comment

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

I wanted to make it clear that some are to be used likeSteps (and hence need a node) while all_the_other_need_not.

//EDIT I have not settled on if this is a good idea hence it is not documented anywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

This would be useful to have written somewhere at the top!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

again i am not even sure if i followed it consequently hence it is not final to stay that way yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, as said this does not have to be resolved within this PR.

$class: 'GitSCM',
branches: scm.branches,
extensions: scm.extensions + [
[$class: 'PerBuildTag'],
Copy link
Contributor

Choose a reason for hiding this comment

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

What do all these extensions do? Can you point to the docu of where you found all these knobs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I got 404. Please link it in the beginning of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you need to login. it is just a collection of available commands on the server. you can use any other jenkins dsl documentation as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

googling jenkinsfile reference brings the full list available via https://jenkins.io/doc/pipeline/steps/

Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why the file is not visible without login?

Yes, such things should be in the intro of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any reason why the file is not visible without login?

No Idea.

we don't link c tutorials in c files, we don't link manpages in our scripts, what makes the Jenkinsfile so different?

Copy link
Contributor

Choose a reason for hiding this comment

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

we don't link c tutorials in c files, we don't link manpages in our scripts,

Because it is not one of the development languages and nevertheless nearly everybody will have to do something with it (with minimal knowledge of the language).

And the Jenkinsfile had little docu even compared to C code. But it slowly improves. 😉

what makes the Jenkinsfile so different?

If I understood you correctly, Jenkinsfile might be written in either groovy or in a DSL. So you need to specify which one we use now.

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 will always be 'groovyish', the choice is between a declarative and a 'scripted' version of the DSL. we currenlty use the scripted one because the declarative has been not complex enough.

However they are to some degree interchangeable.

Because it is not one of the development languages and nevertheless nearly everybody will have to do something with it (with minimal knowledge of the language).

The problem with documenting essential language features is that it is very much a moving target. Patches come out at least once a week.

Linking the documentation that can be found via simple google queries is not providing any benefit, else you would add c tutorials to every c header, or cmake documentation to every cmakelists file.

I would prefer to stop having those documentation discussions till I have finished the accompanying doc/*.md file.

Copy link
Contributor

Choose a reason for hiding this comment

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

it will always be 'groovyish', the choice is between a declarative and a 'scripted' version of the DSL. we currenlty use the scripted one because the declarative has been not complex enough.

Seems like I misunderstood. Please add this information.

Linking the documentation that can be found via simple google queries is not providing any benefit, else you would add c tutorials to every c header, or cmake documentation to every cmakelists file.

No, not every file, only at the entry points. I added them now for cmake 9a7dcbe

you would add c tutorials

There is a huge difference about how well known C and groovy are. What applies to groovy might not apply to C. CMake is similar, though.

I would prefer to stop having those documentation discussions till I have finished the accompanying doc/*.md file.

Ok. But please makes notes of these discussion because the next time I review I will most likely overlook things. And newcomers (speak everyone from the current dev team because afaik nobody else knows these DSLs) will be happy for every pointer that is necessary for them.

[$class: 'RelativeTargetDirectory',
relativeTargetDir: target_dir]
],
userRemoteConfigs: scm.userRemoteConfigs
Copy link
Contributor

Choose a reason for hiding this comment

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

again, what does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see above

sh "git merge --no-ff -m 'merge $VERSION' temp"

sh "dch -l '.$BUILD_NUMBER' 'auto build'"
sh "git commit -am 'auto build $VERSION'"
Copy link
Contributor

Choose a reason for hiding this comment

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

does this work? In shell a single tick would take $VERSION literally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure. It gets expanded before it is passed to the shell by groovy.

@@ -610,13 +664,13 @@ def checksum(file) {

/* Generate a Stage
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 it called maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because it maybe executed. depending on the parameters passed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am referring that this should be documented. And "Generate a Stage" is misleading if it maybe generates a stage.

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 always generating a stage, the stage might just be skipped

checkout scm
docker.image(imageFullName(image)).inside { cl() }
docker.image(imageFullName(image))
.inside("-v ${env.HOME}/git_mirrors:/home/jenkins/git_mirrors") { cl() }
Copy link
Contributor

Choose a reason for hiding this comment

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

Please document this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure what you want documented there. how .inside works is documented in the docker dsl plugin. what the -v switch does to docker run is documented in the docker docs.
and the rest is groovy syntax?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, exactly. There are so many DSLs involved that you cannot expect people to know what this does. Please translate it into English.

I am also curious what twas wrong with the previous statement.

And please link to all these documentations in the very beginning of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there was nothing wrong with the old statement, it is just that we now want the mirror repositories available inside docker containers because we manipulate them for the releases (and we do not want full checkouts).

* General Information about Jenkinsfiles can be found at
* https://jenkins.io/doc/book/pipeline/jenkinsfile/.
*
* A Snipped generators is available to the public at
Copy link
Member

Choose a reason for hiding this comment

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

Typo: SnippedSnippet

Copy link
Contributor

Choose a reason for hiding this comment

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

and singular "generator"?

@markus2330
Copy link
Contributor

@ingwinlu What happened with the jenkinsfile buildjob? "Build failed. Might be due to partial build. Please check status in jenkins". In Jenkins it is yellow.

@ingwinlu
Copy link
Contributor Author

Either due to me trying to get the jenkinsfile to build for pushes to master as well (ghpbr apparently has some problems there). Or the homepage build jobs lags out the master so hard that it can not properly run build jobs.

@ingwinlu
Copy link
Contributor Author

jenkins build jenkinsfile please

@markus2330
Copy link
Contributor

Ok, sorry. I needed a testrun for the homepage (it would be a release stopper, too. But everything worked!)

@ingwinlu
Copy link
Contributor Author

I have the dockerfiles for the front + backend ready to go just waiting to get this merged first

@markus2330 markus2330 merged commit bc3b7b5 into ElektraInitiative:master May 12, 2018
@markus2330
Copy link
Contributor

Thank you, great job!

@markus2330
Copy link
Contributor

@ingwinlu ingwinlu deleted the add_jenkinsfile_package branch May 13, 2018 04:23
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.

build:elektra-git-buildpackage-stretch failing Stretch Debian Packages
4 participants