PR#185: Custom build source method - copr/copr

copr / copr

#185 Custom build source method

Merged 6 years ago by clime. Opened 6 years ago by praiskup.

Unknown source custom-source-method into master

[beaker-tests] test the new custom method

Jakub Kadlčík • 6 years ago

176e161

[cli][doc][frontend][python][rpmbuild] new custom source method

Pavel Raiskup • 6 years ago

35278e9

[frontend] swap arguments in custom webhook and document

Pavel Raiskup • 6 years ago

3867d74

beaker-tests/Sanity/copr-cli-basic-operations/files/quick-package.sh

file added

+49

		`@@ -0,0 +1,49 @@`
		`+ #! /bin/sh -x`
		`+`
		`+ set -e`
		`+`
		`+ generate_specfile()`
		`+ {`
		`+ test -n "$DESTDIR" && mkdir -p "$DESTDIR"`
		`+`
		`+ test -n "$BUILDDEPS" && {`
		`+ for i in $BUILDDEPS; do`
		`+ rpm -q $i`
		`+ done`
		`+ }`
		`+`
		`+ if ${HOOK_PAYLOAD-false}; then`
		`+ test -f hook_payload`
		`+ test "$(cat hook_payload)" = "{\"a\": \"b\"}"`
		`+ else`
		`+ ! test -f hook_payload`
		`+ fi`
		`+`
		`+ cat > "${DESTDIR-.}"/quick-package.spec <<\EOF`
		`+ Name: quick-package`
		`+ Version: 0`
		`+ Release: 0%{?dist}`
		`+ Summary: dummy package`
		`+ License: GPL`
		`+ URL: http://example.com/`
		`+`
		`+ %{!?_pkgdocdir: %global _pkgdocdir %{_docdir}/%{name}-%{version}}`
		`+`
		`+ %description`
		`+ nothing`
		`+`
		`+`
		`+ %install`
		`+ mkdir -p $RPM_BUILD_ROOT/%{_pkgdocdir}`
		`+ echo "this does nothing" > $RPM_BUILD_ROOT/%{_pkgdocdir}/README`
		`+`
		`+`
		`+ %files`
		`+ %doc %{_pkgdocdir}/README`
		`+`
		`+`
		`+ %changelog`
		`+ * Thu Jun 05 2014 Pavel Raiskup <praiskup@redhat.com> - 0-1`
		`+ - does nothing!`
		`+ EOF`
		`+ }`

beaker-tests/Sanity/copr-cli-basic-operations/runtest-custom-method.sh

file added

+216

		`@@ -0,0 +1,216 @@`
		`+ #!/bin/bash`
		`+`
		`+ . /usr/bin/rhts-environment.sh \|\| exit 1`
		`+ . /usr/share/beakerlib/beakerlib.sh \|\| exit 1`
		`+`
		+ export RESULTDIR=`mktemp -d`
		`+`
		`+ export TESTPATH="$( builtin cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"`
		`+`
		`+ export IN=$TESTPATH/action-tasks.json`
		`+ export OUT=$TESTPATH/action-results.out.json`
		`+`
		`+ if [[ ! $FRONTEND_URL ]]; then`
		`+ FRONTEND_URL="http://copr-fe-dev.cloud.fedoraproject.org"`
		`+ fi`
		`+`
		`+`
		`+ NAME_VAR="TEST$(date +%s)"`
		`+`
		`+`
		`+ parse_build_id()`
		`+ {`
		`+ local id`
		`+ id=$(grep 'Created builds:' "$rlRun_LOG" \| sed 's/.* //')`
		`+ test -n "$id" \|\| return 1`
		`+ export BUILD_ID=$id`
		`+ }`
		`+`
		`+ cleanup_resultdir ()`
		`+ (`
		`+ rm -rf "$RESULTDIR/*"`
		`+ )`
		`+`
		`+ check_resultdir ()`
		`+ (`
		`+ set -e`
		`+ cd "$RESULTDIR/fedora-rawhide-x86_64"`
		`+ # @FIXME`
		`+ #test -f "$RESULTDIR"/script`
		`+ for i in $FILES; do`
		`+ echo "checking that $i exists in resultdir"`
		`+ test -f "$i"`
		`+ done`
		`+`
		`+ NV=$1`
		`+`
		`+ echo "checking that only one srpm exists"`
		`+ set -- *.src.rpm`
		`+ test 1 -eq "$#"`
		`+`
		`+ echo "checking that srpm version is fine"`
		`+ case $1 in`
		`+ $NV*.src.rpm) ;; # OK`
		`+ *) false ;;`
		`+ esac`
		`+ )`
		`+`
		`+ check_http_status ()`
		`+ {`
		`+ grep "HTTP/1.1 $1" "$rlRun_LOG"`
		`+ }`
		`+`
		`+ quick_package_script ()`
		`+ {`
		`+ cp "$TESTPATH/files/quick-package.sh" script`
		`+ echo "$1" >> script`
		`+ }`
		`+`
		`+`
		`+ rlJournalStart`
		`+ rlPhaseStartTest Test`
		+ rlRun "export WORKDIR=\`mktemp -d\`"
		`+ rlRun "test -n \"\$WORKDIR\""`
		`+`
		`+ rlRun 'cd "$WORKDIR"'`
		`+ rlRun 'echo workdir: $WORKDIR'`
		`+`
		`+`
		`+ rlLogInfo "Create the project $PROJECT"`
		`+ PROJECT=custom-1-$NAME_VAR`
		`+ rlRun 'copr-cli create "$PROJECT" --chroot fedora-rawhide-x86_64'`
		`+`
		`+`
		`+ rlLogInfo "Test add-package && build"`
		`+ rlRun 'cleanup_resultdir'`
		`+ rlRun 'quick_package_script generate_specfile'`
		`+ rlRun 'copr add-package-custom "$PROJECT" \`
		`+ --name quick-package \`
		`+ --script script \`
		`+ --script-chroot fedora-rawhide-x86_64'`
		`+ rlRun -s 'copr build-package "$PROJECT" --name quick-package --nowait'`
		`+ rlRun 'parse_build_id'`
		`+ rlRun 'copr watch-build $BUILD_ID'`
		`+ rlRun 'copr download-build $BUILD_ID --dest $RESULTDIR'`
		`+ rlRun 'FILES="success" check_resultdir quick-package-0-0'`
		`+`
		`+`
		`+ rlLogInfo "Test edit-package && --resultdir"`
		`+ rlRun 'cleanup_resultdir'`
		`+ rlRun 'quick_package_script "DESTDIR=rrr generate_specfile"'`
		`+ rlRun 'copr edit-package-custom "$PROJECT" \`
		`+ --name quick-package \`
		`+ --script script \`
		`+ --script-chroot fedora-rawhide-x86_64'`
		`+ rlRun -s 'copr build-package "$PROJECT" --name quick-package --nowait'`
		`+ rlRun 'parse_build_id'`
		`+ # Should fail, there's no spec file in expected resultdir=.`
		`+ rlRun 'copr watch-build $BUILD_ID' 4`
		`+`
		`+ rlRun 'copr edit-package-custom "$PROJECT" \`
		`+ --name quick-package \`
		`+ --script script \`
		`+ --script-resultdir rrr \`
		`+ --script-chroot fedora-rawhide-x86_64'`
		`+ rlRun -s 'copr build-package "$PROJECT" --name quick-package --nowait'`
		`+ rlRun 'parse_build_id'`
		`+ rlRun 'copr watch-build $BUILD_ID'`
		`+ rlRun 'copr download-build $BUILD_ID --dest $RESULTDIR'`
		`+ rlRun 'FILES="success" check_resultdir quick-package-0-0'`
		`+`
		`+`
		`+ rlLogInfo "Test that builddeps get propagated"`
		`+ builddeps="automake autoconf spax"`
		`+ rlRun 'cleanup_resultdir'`
		`+ rlRun 'quick_package_script "BUILDDEPS=xxx generate_specfile"'`
		`+ rlRun 'copr edit-package-custom "$PROJECT" \`
		`+ --name quick-package \`
		`+ --script script \`
		`+ --script-builddeps "$builddeps" \`
		`+ --script-chroot fedora-rawhide-x86_64'`
		`+ rlRun -s 'copr build-package "$PROJECT" --name quick-package --nowait'`
		`+ rlRun 'parse_build_id'`
		`+ # Invalid BUILDDEPS value, should fail`
		`+ rlRun 'copr watch-build $BUILD_ID' 4`
		`+`
		`+ rlRun 'quick_package_script "BUILDDEPS=\"$builddeps\" generate_specfile"'`
		`+ rlRun 'copr edit-package-custom "$PROJECT" \`
		`+ --name quick-package \`
		`+ --script script \`
		`+ --script-builddeps "$builddeps" \`
		`+ --script-chroot fedora-rawhide-x86_64'`
		`+ rlRun -s 'copr build-package "$PROJECT" --name quick-package --nowait'`
		`+ rlRun 'parse_build_id'`
		`+ # Invalid BUILDDEPS value, should fail`
		`+ rlRun 'copr watch-build $BUILD_ID'`
		`+ rlRun 'copr download-build $BUILD_ID --dest $RESULTDIR'`
		`+ rlRun 'FILES="success" check_resultdir quick-package-0-0'`
		`+`
		`+`
		`+ rlLogInfo "check that hook_payload get's created"`
		`+ rlRun 'cleanup_resultdir'`
		`+ rlRun 'quick_package_script "HOOK_PAYLOAD=: generate_specfile"'`
		`+ rlRun 'copr edit-package-custom "$PROJECT" \`
		`+ --name quick-package \`
		`+ --script script \`
		`+ --script-chroot fedora-rawhide-x86_64 \`
		`+ --webhook-rebuild on'`
		`+ rlRun -s 'copr build-package "$PROJECT" --name quick-package --nowait'`
		`+ rlRun 'parse_build_id'`
		`+ rlLogInfo "Still should fail, since this build is not triggered by webhook."`
		`+ rlRun 'copr watch-build $BUILD_ID' 4`
		`+`
		`+ trigger_url="$FRONTEND_URL/webhooks/custom/$copr_id/webhook_secret/quick-package/"`
		`+ rlRun -s 'curl -I "$trigger_url"' 0 # GET can't work`
		`+ rlRun 'check_http_status 405'`
		`+`
		`+ content_type_option=' -H "Content-Type: application/json"'`
		`+ data_option=' --data '\''{"a": "b"}'\'`
		`+`
		`+ rlLogInfo "full cmd would be: curl -X POST $content_type_option $data_option $trigger_url"`
		`+ rlRun "build_id=\$(curl -X POST $data_option \"$trigger_url\")" 0`
		`+ rlLogInfo "Still fails since the POST data are not json"`
		`+ rlRun 'copr watch-build $BUILD_ID' 4`
		`+`
		`+ rlLogInfo "Still fails since the POST data are not json"`
		`+ rlRun "build_id=\$(curl -X POST $content_type_option $data_option \"$trigger_url\")" 0`
		`+ rlLogInfo "Should succeed finally"`
		`+ # @FIXME`
		`+ # rlRun 'copr watch-build $build_id'`
		`+ # rlRun 'copr download-build $build_id --dest $RESULTDIR'`
		`+ # rlRun 'FILES="success" check_resultdir quick-package-0-0'`
		`+`
		`+`
		`+ rlLogInfo "basic buildcustom command, with fedora-latest-x86_64 chroot (default)"`
		`+ rlRun 'cleanup_resultdir'`
		`+ rlRun 'quick_package_script "generate_specfile"'`
		`+ rlRun -s "copr buildcustom $PROJECT --script script --nowait"`
		`+ rlRun 'parse_build_id'`
		`+ rlRun 'copr watch-build $BUILD_ID'`
		`+ rlRun 'copr download-build $BUILD_ID --dest $RESULTDIR'`
		`+ rlRun 'FILES="success" check_resultdir quick-package-0-0'`
		`+`
		`+`
		`+ rlLogInfo "buildcustom with --builddeps"`
		`+ builddeps='postgresql-devel'`
		`+ rlRun 'cleanup_resultdir'`
		`+ rlRun "quick_package_script 'BUILDDEPS=\"$builddeps\" generate_specfile'"`
		`+ rlRun -s "copr buildcustom $PROJECT --script script --script-builddeps \"$builddeps\" --nowait"`
		`+ rlRun 'parse_build_id'`
		`+ rlRun 'copr watch-build $BUILD_ID'`
		`+ rlRun 'copr download-build $BUILD_ID --dest $RESULTDIR'`
		`+ rlRun 'FILES="success" check_resultdir quick-package-0-0'`
		`+`
		`+`
		`+ rlLogInfo "buildcustom with --builddeps and --resultdir"`
		`+ destdir=abc`
		`+ rlRun 'cleanup_resultdir'`
		`+ rlRun "quick_package_script 'BUILDDEPS=\"$builddeps\" DESTDIR=$destdir generate_specfile'"`
		`+ rlRun -s "copr buildcustom $PROJECT --script script --script-resultdir=$destdir --script-builddeps \"$builddeps\" --nowait"`
		`+ rlRun 'parse_build_id'`
		`+ rlRun 'copr watch-build $BUILD_ID'`
		`+ rlRun 'copr download-build $BUILD_ID --dest $RESULTDIR'`
		`+ rlRun 'FILES="success" check_resultdir quick-package-0-0'`
		`+`
		`+ rlPhaseEnd`
		`+ rlJournalEnd`

cli/copr_cli/main.py

file modified

+71

		`@@ -270,6 +270,21 @@`
		`return self.process_build(args, self.client.create_new_build_rubygems, data)`

		`@requires_api_auth`
		`+ def action_build_custom(self, args):`
		`+ """`
		`+ Method called when 'buildcustom' has been selected by the user.`
		`+`
		`+ :param args: argparse arguments provided by the user`
		`+ """`
		`+ data = {`
		`+ 'script': ''.join(args.script.readlines()),`
		`+ }`
		`+ for arg in ['script_chroot', 'script_builddeps',`
		`+ 'script_resultdir']:`
		`+ data[arg] = getattr(args, arg)`
		`+ return self.process_build(args, self.client.create_new_build_custom, data)`
		`+`
		`+ @requires_api_auth`
		`def action_build_distgit(self, args):`
		`"""`
		`Method called when the 'buildfedpkg' action has been selected by the user.`
		`@@ -556,6 +571,23 @@`
		`result = self.client.edit_package_rubygems(ownername=ownername, projectname=projectname, **data)`
		`print(result.message)`

		`+ @requires_api_auth`
		`+ def action_add_or_edit_package_custom(self, args):`
		`+ ownername, projectname = parse_name(args.copr)`
		`+ data = {`
		`+ "package_name": args.name,`
		`+ "script": ''.join(args.script.readlines()),`
		`+ "script_chroot": args.script_chroot,`
		`+ "script_builddeps": args.script_builddeps,`
		`+ "script_resultdir": args.script_resultdir,`
		`+ "webhook_rebuild": ON_OFF_MAP[args.webhook_rebuild],`
		`+ }`
		`+ if args.create:`
		`+ result = self.client.add_package_custom(ownername=ownername, projectname=projectname, **data)`
		`+ else:`
		`+ result = self.client.edit_package_custom(ownername=ownername, projectname=projectname, **data)`
		`+ print(result.message)`
		`+`
		`def action_list_packages(self, args):`
		`ownername, projectname = parse_name(args.copr)`
		`data = {`
		`@@ -810,6 +842,22 @@`
		`parser_distgit_args_parent.add_argument("--branch", metavar="BRANCH", dest="branch",`
		`help="Specify branch to be used")`

		`+ parser_custom_args_parent = argparse.ArgumentParser(add_help=False)`
		`+ parser_custom_args_parent.add_argument(`
		`+ '--script', required=True,`
		`+ type=argparse.FileType('r'),`
		`+ help='text file (script) to be used to prepare the sources')`
		`+ parser_custom_args_parent.add_argument(`
		`+ '--script-chroot',`
		`+ help='mock chroot to build sources for the SRPM in')`
		`+ parser_custom_args_parent.add_argument(`
		`+ '--script-builddeps',`
		`+ help='space separated list of packages needed to build the sources')`
		`+ parser_custom_args_parent.add_argument(`
		`+ '--script-resultdir',`
		`+ help='where SCRIPT generates the result, relatively to script\'s '`
		`+ '$PWD (defaults to \'.\')')`
		`+`
		`#########################################################`
		`### Build options ###`
		`#########################################################`
		`@@ -846,6 +894,13 @@`
		`help="Build gem from rubygems.org to a specified copr")`
		`parser_build_rubygems.set_defaults(func="action_build_rubygems")`

		`+ # create the parser for the "buildcustom" command`
		`+ parser_build_custom = subparsers.add_parser(`
		`+ "buildcustom",`
		`+ parents=[parser_custom_args_parent, parser_build_parent],`
		`+ help="Build packages from SRPM generated by custom script")`
		`+ parser_build_custom.set_defaults(func="action_build_custom")`
		`+`
		`# create the parser for the "buildfedpkg" command`
		`parser_build_distgit = subparsers.add_parser("buildfedpkg", parents=[parser_distgit_args_parent, parser_build_parent],`
		`help="DEPRECATED. Use SCM source type instead.")`
		`@@ -991,6 +1046,22 @@`
		`parents=[parser_rubygems_args_parent, parser_add_or_edit_package_parent])`
		`parser_edit_package_rubygems.set_defaults(func="action_add_or_edit_package_rubygems", create=False)`

		`+ # Custom build method - edit/create package`
		`+ parser_add_package_custom = subparsers.add_parser(`
		`+ "add-package-custom",`
		`+ help="Creates a new package where sources are built by custom script",`
		`+ parents=[parser_custom_args_parent, parser_add_or_edit_package_parent])`
		`+ parser_add_package_custom.set_defaults(`
		`+ func="action_add_or_edit_package_custom",`
		`+ create=True)`
		`+ parser_edit_package_custom = subparsers.add_parser(`
		`+ "edit-package-custom",`
		`+ help="Edits an existing Custom package",`
		`+ parents=[parser_custom_args_parent, parser_add_or_edit_package_parent])`
		`+ parser_edit_package_custom.set_defaults(`
		`+ func="action_add_or_edit_package_custom",`
		`+ create=False)`
		`+`
		`# package listing`
		`parser_list_packages = subparsers.add_parser("list-packages",`
		`help="Returns list of packages in the given copr")`

doc/custom_source_method.rst

file added

+130

		`@@ -0,0 +1,130 @@`
		`+ .. _custom_source_method:`
		`+`
		`+ Coustom source method`
		`+ =====================`
		`+`
		`+ Build sources (for SRPM) by user-defined script.`
		`+`
		`+ The idea behind the script is simple, when the script is run - it's only`
		`+ mandatory output is specfile, plus optionally any other file needed to`
		`+ successfully build a source RPM from that spec file (usually tarball(s),`
		`+ patches, etc.). By default we expect that the script generates the files in`
		`+ current working directory (resultdir='.').`
		`+`
		`+ Having turing-complete powers and Internet access - the script can do basically`
		`+ anything to get all the source pieces together. The only limitation is that it`
		`+ is executed under non-privileged user (the script is executed in mock chroot,`
		`+ under 'mockbuild' user). This brings one major obstacle that you can not`
		`+ install any RPM packages from within the script; if you need to have some`
		`+ packages pre-installed, you need to specify them "declaratively" as a list of`
		`+ (srpm)build-dependencies. The reasons for this design are that (a) it is easier`
		`+ and and safer to develop scripts which don't require admin access, (b) it is`
		`+ convenient to write "portable" scripts (even though the script is executed in`
		`+ rpm-based mock chroot, the script itself can be easily written/tested on e.g.`
		`+ Gentoo) and (c) it follows the usual workflows of maintainers (install packages`
		`+ under root, and work the rest of the day as non-root) and mock workflow.`
		`+`
		`+`
		`+ Required configuration for custom method`
		`+ ----------------------------------------`
		`+`
		`+ Basically you only have to specify script and chroot parameter.`
		`+`
		`+ - script - scipt file content; written in any scripting language, but pay`
		+ attention to specify shebang properly, e.g. `#! /bin/sh` for (posix) shell
		`+ scripts. Also note that the interpreter might not be available by default,`
		`+ you might have to request its installation via builddeps argument).`
		`+`
		`+ - chroot - (mock) chroot where the script is executed. By default, the`
		+ `fedora-latest-x86_64` chroot is used, which represents the latest stable
		+ or `branched <https://fedoraproject.org/wiki/Releases/Branched>`_ Fedora
		`+ version available in Copr at the time of the build request (e.g.`
		+ `fedora-27-x86_64` when `fedora-rawhide-x86_64` represents Fedora 28).
		`+`
		`+`
		`+ Optional parameters`
		`+ -------------------`
		`+`
		`+ - builddeps - space-separated list of packages which are pre-installed into`
		`+ the build chroot (before the script is executed).`
		`+`
		`+ - resultdir - where the script generates its output. By default, it is`
		`+ assumed to be current working directory.`
		`+`
		`+`
		`+ Webhook support`
		`+ ---------------`
		`+`
		`+ The only useful webhook for custom source method is the custom web-hook.`
		`+ Because unlike other methods, custom method implementation doesn't itself pay`
		`+ attention to webhook payload (json data used e.g. by GitLab to indicate what`
		`+ type of event triggered the webhok call) nor there's any particular "clone url".`
		`+`
		`+ With custom webhook, the payload parsing/analysis is left to the script (in`
		`+ other words it is user's responsibility). For that purpose custom webhook`
		+ handler dumps the webhook payload (if any) into file `$PWD/hook_payload` file
		`+ (from the script POV).`
		`+`
		`+ For example, if GitLab's merge-request event from contributor/project.git to`
		`+ owner/repo.git "calls" the custom webhook for package foo, the script`
		+ has to parse the `hook_payload` file to detect the fact that
		`+ contributor/project.git should be cloned (instead of owner/repo.git) to`
		`+ generate the sources.`
		`+`
		`+ Since this all is in user's hands, it is not technically incorrect to have empty`
		+ hook payload, e.g. it is valid to call `curl -X POST <THE_CUSTOM_HOOK_URL>` to
		`+ trigger the custom source build method.`
		`+`
		`+`
		`+ Examples`
		`+ --------`
		`+`
		`+ - Trivial example (only spec file)::`
		`+`
		`+ $ cat script`
		`+ #! /bin/sh -x`
		`+ curl https://praiskup.fedorapeople.org/quick-package.spec -O`
		`+`
		`+ $ copr add-package-custom PROJECT \`
		`+ --name quick-package \`
		`+ --script script`
		`+`
		`+ $ copr build-package --name quick-package PROJECT # trigger the build`
		`+`
		`+`
		`+ - Simple example with Python package with git submodules and in-tree sources::`
		`+`
		`+ $ cat script`
		`+ #! /bin/sh`
		`+`
		`+ mkdir -p results`
		`+ resultdir=$(readlink -f results)`
		`+`
		`+ set -x # verbose output`
		`+ set -e # fail the whole script if some command fails`
		`+`
		`+ # obtain the source code`
		`+ git clone https://github.com/praiskup/resalloc --recursive --depth 1`
		`+ cd resalloc`
		`+`
		`+ # 1. generate source tarball into resultdir`
		`+ python setup.py sdist -d "$resultdir"`
		`+`
		`+ # 2. copy the spec file into resultdir, change the release number so each build`
		`+ # has unique name-version-release triplet`
		`+ cd rpm`
		`+ release='~'$(date +"%Y%m%d_%H%M%S")`
		`+ sed "s/$^Release:[[:space:]]*[[:digit:]]\+$/\1$release/" resalloc.spec \`
		`+ > "$resultdir"/resalloc.spec`
		`+`
		`+ # 3. copy other sources`
		`+ cp *.service "$resultdir"`
		`+`
		`+ $ copr add-package-custom PROJECT \`
		`+ --name resalloc \`
		`+ --script script \`
		`+ --script-resultdir results \`
		`+ --script-builddeps 'git' \`
		`+ --script-chroot fedora-rawhide-x86_64`
		`+`
		`+ $ copr build-package --name resalloc PROJECT # trigger the build`

doc/user_documentation.rst

file modified

		`@@ -115,6 +115,14 @@`
		here is `gem2rpm <https://github.com/fedora-ruby/gem2rpm>`_.


		`+ Custom (script)`
		`+ ^^^^^^^^^^^^^^^`
		`+`
		`+ This source type uses a user-defined script to generate sources (which are later`
		`+ used to create SRPM). For more info, have a look at`
		+ :ref:`custom_source_method`.
		`+`
		`+`
		`GitHub Webhooks`
		`---------------`

frontend/coprs_frontend/coprs/filters.py

file modified

		`@@ -220,6 +220,7 @@`
		`"scm": "Build from an SCM repository",`
		`"pypi": "Build from PyPI",`
		`"rubygems": "Build from RubyGems",`
		`+ "custom": "Custom build method",`
		`}`

		`return description_map.get(state, "")`

frontend/coprs_frontend/coprs/forms.py

file modified

+61

		`@@ -41,6 +41,8 @@`
		`return PackageFormTito # deprecated`
		`elif source_type_text == 'mock_scm':`
		`return PackageFormMock # deprecated`
		`+ elif source_type_text == "custom":`
		`+ return PackageFormCustom`
		`else:`
		`raise exceptions.UnknownSourceTypeException("Invalid source type")`

		`@@ -574,6 +576,60 @@`
		`})`


		`+ class PackageFormCustom(BasePackageForm):`
		`+ script = wtforms.TextAreaField(`
		`+ "Script",`
		`+ validators=[`
		`+ wtforms.validators.DataRequired(),`
		`+ wtforms.validators.Length(`
		`+ max=4096,`
		`+ message="Maximum script size is 4kB"),`
		`+ ],`
		`+ )`
		`+`
		`+ builddeps = wtforms.StringField(`
		`+ "Build dependencies",`
		`+ validators=[wtforms.validators.Optional()])`
		`+`
		`+ chroot = wtforms.SelectField(`
		`+ 'Mock chroot',`
		`+ choices=[],`
		`+ default='fedora-latest-x86_64',`
		`+ )`
		`+`
		`+ resultdir = wtforms.StringField(`
		`+ "Result directory",`
		`+ validators=[wtforms.validators.Optional()])`
		`+`
		`+ def __init__(self, args, *kwargs):`
		`+ super(PackageFormCustom, self).__init__(args, *kwargs)`
		`+ chroot_objects = models.MockChroot.query.filter(models.MockChroot.is_active).all()`
		`+`
		`+ chroots = [c.name for c in chroot_objects]`
		`+ chroots.sort()`
		`+ chroots = [(name, name) for name in chroots]`
		`+`
		`+ arches = set()`
		`+ for ch in chroot_objects:`
		`+ if ch.os_release == 'fedora':`
		`+ arches.add(ch.arch)`
		`+`
		`+ self.chroot.choices = []`
		`+ if arches:`
		`+ self.chroot.choices += [('fedora-latest-' + l, 'fedora-latest-' + l) for l in arches]`
		`+`
		`+ self.chroot.choices += chroots`
		`+`
		`+ @property`
		`+ def source_json(self):`
		`+ return json.dumps({`
		`+ "script": self.script.data,`
		`+ "chroot": self.chroot.data,`
		`+ "builddeps": self.builddeps.data,`
		`+ "resultdir": self.resultdir.data,`
		`+ })`
		`+`
		`+`
		`class RebuildAllPackagesFormFactory(object):`
		`def __new__(cls, active_chroots, package_names):`
		`form_cls = BaseBuildFormFactory(active_chroots, FlaskForm)`
		`@@ -677,6 +733,11 @@`
		`return form`


		`+ class BuildFormCustomFactory(object):`
		`+ def __new__(cls, active_chroots):`
		`+ return BaseBuildFormFactory(active_chroots, PackageFormCustom)`
		`+`
		`+`
		`class BuildFormUrlFactory(object):`
		`def __new__(cls, active_chroots):`
		`form = BaseBuildFormFactory(active_chroots, FlaskForm)`

frontend/coprs_frontend/coprs/helpers.py

file modified

		`@@ -114,6 +114,7 @@`
		`"pypi": 5, # package_name, version, python_versions`
		`"rubygems": 6, # gem_name`
		`"scm": 8, # type, clone_url, committish, subdirectory, spec, srpm_build_method`
		`+ "custom": 9, # user-provided script to build sources`
		`}`

		`# The same enum is also in distgit's helpers.py`

frontend/coprs_frontend/coprs/logic/builds_logic.py

file modified

+25

		`@@ -483,6 +483,31 @@`
		`return cls.create_new(user, copr, source_type, source_json, chroot_names, **build_options)`

		`@classmethod`
		`+ def create_new_from_custom(cls, user, copr,`
		`+ script, script_chroot=None, script_builddeps=None,`
		`+ script_resultdir=None, chroot_names=None, **kwargs):`
		`+ """`
		`+ :type user: models.User`
		`+ :type copr: models.Copr`
		`+ :type script: str`
		`+ :type script_chroot: str`
		`+ :type script_builddeps: str`
		`+ :type script_resultdir: str`
		`+ :type chroot_names: List[str]`
		`+ :rtype: models.Build`
		`+ """`
		`+ source_type = helpers.BuildSourceEnum("custom")`
		`+ source_dict = {`
		`+ 'script': script,`
		`+ 'chroot': script_chroot,`
		`+ 'builddeps': script_builddeps,`
		`+ 'resultdir': script_resultdir,`
		`+ }`
		`+`
		`+ return cls.create_new(user, copr, source_type, json.dumps(source_dict),`
		`+ chroot_names, **kwargs)`
		`+`
		`+ @classmethod`
		`def create_new_from_upload(cls, user, copr, f_uploader, orig_filename,`
		`chroot_names=None, **build_options):`
		`"""`

frontend/coprs_frontend/coprs/models.py

file modified

+21

		`@@ -505,6 +505,17 @@`
		`db.Index('build_order', "is_background", "id"),`
		`db.Index('build_filter', "source_type", "canceled"))`

		`+ def __init__(self, args, *kwargs):`
		`+ if kwargs.get('source_type') == helpers.BuildSourceEnum("custom"):`
		`+ source_dict = json.loads(kwargs['source_json'])`
		`+ if 'fedora-latest' in source_dict['chroot']:`
		`+ arch = source_dict['chroot'].split('-')[2]`
		`+ source_dict['chroot'] = \`
		`+ MockChroot.latest_fedora_branched_chroot(arch=arch).name`
		`+ kwargs['source_json'] = json.dumps(source_dict)`
		`+`
		`+ super(Build, self).__init__(args, *kwargs)`
		`+`
		`id = db.Column(db.Integer, primary_key=True)`
		`# single url to the source rpm, should not contain " ", "\n", "\t"`
		`pkgs = db.Column(db.Text)`
		`@@ -834,6 +845,16 @@`
		`distgit_branch = db.relationship("DistGitBranch",`
		`backref=db.backref("chroots"))`

		`+ @classmethod`
		`+ def latest_fedora_branched_chroot(cls, arch='x86_64'):`
		`+ return (cls.query`
		`+ .filter(cls.is_active == True)`
		`+ .filter(cls.os_release == 'fedora')`
		`+ .filter(cls.os_version != 'rawhide')`
		`+ .filter(cls.arch == arch)`
		`+ .order_by(cls.os_version.desc())`
		`+ .first())`
		`+`
		`@property`
		`def name(self):`
		`"""`

frontend/coprs_frontend/coprs/templates/coprs/detail/_builds_forms.html

file modified

		`@@ -1,4 +1,5 @@`
		`{% from "_helpers.html" import render_field, render_form_errors, copr_url, render_pypi_python_versions_field, render_additional_build_options, render_srpm_build_method_box %}`
		`+ {% from "coprs/detail/_method_forms.html" import copr_method_form_fileds_custom %}`

		`{# This file contains forms for the "New Build" action`

		`@@ -156,6 +157,14 @@`
		`{% endmacro %}`


		`+ {% macro copr_build_form_custom(form, view, copr) %}`
		`+ {{ copr_build_form_begin(form, view, copr) }}`
		`+ {{ source_description('Provide custom script to build sources.')}}`
		`+ {{ copr_method_form_fileds_custom(form) }}`
		`+ {{ copr_build_form_end(form, view, copr) }}`
		`+ {% endmacro %}`
		`+`
		`+`
		`{% macro copr_build_form_rebuild(form, view, copr, build) %}`
		`{{ copr_build_form_begin(form, view, copr, build, hide_panels=True) }}`
		`{{ copr_build_form_end(form, view, copr, hide_panels=True) }}`

frontend/coprs_frontend/coprs/templates/coprs/detail/_method_forms.html

file added

+22

		`@@ -0,0 +1,22 @@`
		`+ {% from "_helpers.html" import render_field %}`
		`+ {% macro copr_method_form_fileds_custom(form) %}`
		`+ {{ render_field(`
		`+ form.script,`
		`+ rows=5,`
		`+ style='font-family: monospace;',`
		`+ placeholder="""#! /bin/sh -x`
		`+ curl https://example.com/package.spec -O`
		`+ curl https://example.com/tarball.tar.gz -O`
		`+ """,`
		`+ info="write a script that generates spec and sources... (Internet ON, non-root UID)"`
		`+ )}}`
		`+ {{ render_field(`
		`+ form.chroot,`
		`+ placeholder="fedora-latest-x86_64",`
		`+ info="what chroot to run the <strong>script</strong> in") }}`
		`+ {{ render_field(form.builddeps, placeholder="Optional - space-separated list of packages",`
		`+ info="packages that the <strong>script</strong> requires for its execution" ) }}`
		`+ {{ render_field(form.resultdir, placeholder="Optional - directory where SCRIPT generates sources",`
		`+ info="path relative to the <strong>script</strong>'s current`
		`+ working directory") }}`
		`+ {% endmacro %}`

frontend/coprs_frontend/coprs/templates/coprs/detail/_package_forms.html

file modified

+10

		`@@ -1,4 +1,5 @@`
		`{% from "_helpers.html" import render_field, render_form_errors, copr_url, render_pypi_python_versions_field, render_additional_build_options, render_srpm_build_method_box %}`
		`+ {% from "coprs/detail/_method_forms.html" import copr_method_form_fileds_custom %}`

		`{% macro copr_package_form_begin(form, view, copr, package) %}`
		`{{ render_form_errors(form) }}`
		`@@ -86,6 +87,15 @@`
		`{{ copr_package_form_end(form, package, 'rubygems') }}`
		`{% endmacro %}`

		`+`
		`+ {% macro copr_package_form_custom(form, view, copr, package) %}`
		`+ {{ copr_package_form_begin(form, view, copr, package) }}`
		`+ {{ copr_method_form_fileds_custom(form) }}`
		`+ {{ render_webhook_rebuild(form) }}`
		`+ {{ copr_package_form_end(form, package, 'custom') }}`
		`+ {% endmacro %}`
		`+`
		`+`
		`{% macro copr_package_form_scm(form, view, copr, package) %}`
		`{{ copr_package_form_begin(form, view, copr, package) }}`

frontend/coprs_frontend/coprs/templates/coprs/detail/_package_helpers.html

file modified

		`@@ -3,6 +3,7 @@`
		`copr_package_form_scm,`
		`copr_package_form_pypi,`
		`copr_package_form_rubygems,`
		`+ copr_package_form_custom,`
		`with context %}`


		`@@ -20,6 +21,7 @@`
		`{{ nav_element("scm", "SCM", copr_url(view, copr, source_type_text="scm", **kwargs)) }}`
		`{{ nav_element("pypi", "PyPI", copr_url(view, copr, source_type_text="pypi", **kwargs)) }}`
		`{{ nav_element("rubygems", "RubyGems", copr_url(view, copr, source_type_text="rubygems", **kwargs)) }}`
		`+ {{ nav_element("custom", "Custom", copr_url(view, copr, source_type_text="custom", **kwargs)) }}`
		`</ul>`
		`{% endmacro %}`

		`@@ -33,6 +35,9 @@`
		`{% elif source_type_text == "rubygems" %}`
		`{{ copr_package_form_rubygems(form_rubygems, view, copr, package) }}`

		`+ {% elif source_type_text == "custom" %}`
		`+ {{ copr_package_form_custom(form_custom, view, copr, package) }}`
		`+`
		`{% else %}`
		`<h3>Wrong source type</h3>`
		`{% endif %}`

frontend/coprs_frontend/coprs/templates/coprs/detail/add_build.html

file modified

		`@@ -38,6 +38,7 @@`
		`{{ nav_element("scm", "SCM", copr_url('coprs_ns.copr_add_build_scm', copr)) }}`
		`{{ nav_element("pypi", "PyPI", copr_url('coprs_ns.copr_add_build_pypi', copr)) }}`
		`{{ nav_element("rubygems", "RubyGems", copr_url('coprs_ns.copr_add_build_rubygems', copr)) }}`
		`+ {{ nav_element("custom", "Custom", copr_url('coprs_ns.copr_add_build_custom', copr)) }}`
		`</ul>`
		`</div>`
		`</div>`

frontend/coprs_frontend/coprs/templates/coprs/detail/add_build/custom.html

file added

+11

		`@@ -0,0 +1,11 @@`
		`+ {% extends "coprs/detail/add_build.html" %}`
		`+`
		`+ {% from "coprs/detail/_builds_forms.html" import copr_build_form_custom with context %}`
		`+`
		`+ {% set add_build_tab = "custom" %}`
		`+`
		`+ {% block build_form %}`
		`+`
		`+ {{ copr_build_form_custom(form, view, copr) }}`
		`+`
		`+ {% endblock %}`

frontend/coprs_frontend/coprs/templates/coprs/detail/settings/webhooks.html

file modified

+14

		`@@ -65,6 +65,20 @@`
		`<li> Select to trigger on <code>Repository Push</code>. </li>`
		`<li> Click the <code>Save</code> button. </li>`
		`</ol>`
		`+`
		`+ <h3> Custom webhook </h3>`
		`+ <div class="well well-sm">`
		`+ {{ custom_url }}`
		`+ </div>`
		`+ <h4> How to use it: </h4>`
		`+ <p> Use the GitLab/GitHub steps above (when needed), or simply </p>`
		`+ <p>`
		`+ <div class="well well-sm">`
		`+ $ curl -X POST {{ custom_url }}`
		`+ </div>`
		`+ Note that the package of name 'PACKAGE_NAME' must exist within this`
		`+ project, and that the 'POST' http method must be specified.`
		`+ </p>`
		`</div>`
		`</div>`

frontend/coprs_frontend/coprs/views/api_ns/api_general.py

file modified

+19

		`@@ -483,6 +483,25 @@`
		`return process_creating_new_build(copr, form, create_new_build)`


		`+ @api_ns.route("/coprs/<username>/<coprname>/new_build_custom/", methods=["POST"])`
		`+ @api_login_required`
		`+ @api_req_with_copr`
		`+ def copr_new_build_custom(copr):`
		`+ form = forms.BuildFormCustomFactory(copr.active_chroots)(csrf_enabled=False)`
		`+ def create_new_build():`
		`+ return BuildsLogic.create_new_from_custom(`
		`+ flask.g.user,`
		`+ copr,`
		`+ form.script.data,`
		`+ form.chroot.data,`
		`+ form.builddeps.data,`
		`+ form.resultdir.data,`
		`+ chroot_names=form.selected_chroots,`
		`+ background=form.background.data,`
		`+ )`
		`+ return process_creating_new_build(copr, form, create_new_build)`
		`+`
		`+`
		`@api_ns.route("/coprs/<username>/<coprname>/new_build_scm/", methods=["POST"])`
		`@api_login_required`
		`@api_req_with_copr`

frontend/coprs_frontend/coprs/views/coprs_ns/coprs_builds.py

file modified

+47

		`@@ -5,6 +5,7 @@`
		`import shutil`
		`import tempfile`

		`+ from functools import wraps`
		`from werkzeug import secure_filename`

		`from coprs import app`
		`@@ -351,6 +352,52 @@`
		`form = forms.BuildFormRubyGemsFactory(copr.active_chroots)()`
		`return process_new_build(copr, form, factory, render_add_build_rubygems, add_view, url_on_success)`

		`+ ############################### Custom builds ###############################`
		`+`
		`+ @coprs_ns.route("/g/<group_name>/<coprname>/new_build_custom/", methods=["POST"])`
		`+ @coprs_ns.route("/<username>/<coprname>/new_build_custom/", methods=["POST"])`
		`+ @login_required`
		`+ @req_with_copr`
		`+ def copr_new_build_custom(copr):`
		`+ """ Handle the build request and redirect back. """`
		`+`
		`+ # TODO: parametric decorator for this view && url_on_success`
		`+ view = 'coprs_ns.copr_new_build_custom'`
		`+ url_on_success = helpers.copr_url('coprs_ns.copr_add_build_custom', copr)`
		`+`
		`+ def factory(**build_options):`
		`+ BuildsLogic.create_new_from_custom(`
		`+ flask.g.user,`
		`+ copr,`
		`+ form.script.data,`
		`+ form.chroot.data,`
		`+ form.builddeps.data,`
		`+ form.resultdir.data,`
		`+ chroot_names=form.selected_chroots,`
		`+ **build_options`
		`+ )`
		`+`
		`+ form = forms.BuildFormCustomFactory(copr.active_chroots)()`
		`+`
		`+ return process_new_build(copr, form, factory, render_add_build_custom,`
		`+ view, url_on_success)`
		`+`
		`+`
		`+`
		`+ @coprs_ns.route("/g/<group_name>/<coprname>/add_build_custom/")`
		`+ @coprs_ns.route("/<username>/<coprname>/add_build_custom/")`
		`+ @login_required`
		`+ @req_with_copr`
		`+ def copr_add_build_custom(copr, form=None):`
		`+ return render_add_build_custom(copr, form,`
		`+ 'coprs_ns.copr_new_build_custom')`
		`+`
		`+ def render_add_build_custom(copr, form, view, package=None):`
		`+ if not form:`
		`+ form = forms.BuildFormCustomFactory(copr.active_chroots)()`
		`+ return flask.render_template("coprs/detail/add_build/custom.html",`
		`+ copr=copr, form=form, view=view)`
		`+`

		`################################ Upload builds ################################`

frontend/coprs_frontend/coprs/views/coprs_ns/coprs_general.py

file modified

+6 -1

		`@@ -422,10 +422,15 @@`
		`copr.id,`
		`copr.webhook_secret)`

		`+ custom_url = "https://{}/webhooks/custom/{}/{}/".format(`
		`+ app.config["PUBLIC_COPR_HOSTNAME"],`
		`+ copr.id,`
		`+ copr.webhook_secret) + "<PACKAGE_NAME>/"`
		`+`
		`return flask.render_template(`
		`"coprs/detail/settings/webhooks.html",`
		`copr=copr, bitbucket_url=bitbucket_url, github_url=github_url,`
		`- gitlab_url=gitlab_url)`
		`+ gitlab_url=gitlab_url, custom_url=custom_url)`


		`@coprs_ns.route("/g/<group_name>/<coprname>/webhooks/")`

frontend/coprs_frontend/coprs/views/coprs_ns/coprs_packages.py

file modified

+11 -3

		`@@ -8,7 +8,7 @@`
		`from coprs import helpers`
		`from coprs.models import Package, Build`
		`from coprs.views.coprs_ns import coprs_ns`
		`- from coprs.views.coprs_ns.coprs_builds import render_add_build_scm, render_add_build_pypi`
		`+ from coprs.views.coprs_ns.coprs_builds import render_add_build_scm, render_add_build_pypi, render_add_build_custom`
		`from coprs.views.misc import login_required, page_not_found, req_with_copr, req_with_copr`
		`from coprs.logic.complex_logic import ComplexLogic`
		`from coprs.logic.packages_logic import PackagesLogic`
		`@@ -107,6 +107,10 @@`
		`form = forms.BuildFormPyPIFactory`
		`f = render_add_build_pypi`
		`view_suffix = "_pypi"`
		`+ elif package.source_type_text == "custom":`
		`+ form = forms.BuildFormCustomFactory`
		`+ f = render_add_build_custom`
		`+ view_suffix = "_custom"`
		`else:`
		`flask.flash("Package {} has not the default source which is required for rebuild. Please configure some source"`
		`.format(package_name, copr.full_name))`
		`@@ -127,6 +131,7 @@`
		`"scm": forms.PackageFormScm(),`
		`"pypi": forms.PackageFormPyPI(),`
		`"rubygems": forms.PackageFormRubyGems(),`
		`+ "custom": forms.PackageFormCustom(),`
		`}`

		`if "form" in kwargs:`
		`@@ -135,7 +140,8 @@`
		`return flask.render_template("coprs/detail/add_package.html", copr=copr, package=None,`
		`source_type_text=source_type_text, view="coprs_ns.copr_new_package",`
		`form_scm=form["scm"], form_pypi=form["pypi"],`
		`- form_rubygems=form["rubygems"])`
		`+ form_rubygems=form["rubygems"],`
		`+ form_custom=form['custom'])`


		`@coprs_ns.route("/<username>/<coprname>/package/new/<source_type_text>", methods=["POST"])`
		`@@ -167,6 +173,7 @@`
		`"scm": forms.PackageFormScm,`
		`"pypi": forms.PackageFormPyPI,`
		`"rubygems": forms.PackageFormRubyGems,`
		`+ "custom": forms.PackageFormCustom,`
		`}`
		`form = {k: v(formdata=None) for k, v in form_classes.items()}`

		`@@ -178,7 +185,8 @@`
		`return flask.render_template("coprs/detail/edit_package.html", package=package, copr=copr,`
		`source_type_text=source_type_text, view="coprs_ns.copr_edit_package",`
		`form_scm=form["scm"], form_pypi=form["pypi"],`
		`- form_rubygems=form["rubygems"])`
		`+ form_rubygems=form["rubygems"],`
		`+ form_custom=form['custom'])`


		`@coprs_ns.route("/<username>/<coprname>/package/<package_name>/edit/<source_type_text>", methods=["POST"])`

frontend/coprs_frontend/coprs/views/webhooks_ns/webhooks_general.py

file modified

+2 -2

		`@@ -226,8 +226,8 @@`
		`shutil.rmtree(self.tmp)`


		`- @webhooks_ns.route("/custom/<uuid>/<copr_id>/", methods=["POST"])`
		`- @webhooks_ns.route("/custom/<uuid>/<copr_id>/<package_name>/", methods=["POST"])`
		`+ @webhooks_ns.route("/custom/<copr_id>/<uuid>/", methods=["POST"])`
		`+ @webhooks_ns.route("/custom/<copr_id>/<uuid>/<package_name>/", methods=["POST"])`
		`@copr_id_and_uuid_required`
		`@package_name_required`
		`@skip_invalid_calls`

frontend/coprs_frontend/tests/test_webhooks.py

file modified

+1 -1

		`@@ -6,7 +6,7 @@`

		`class TestCustomWebhook(CoprsTestCase):`
		`def custom_post(self, data, token, copr_id, package_name=None):`
		`- url = "/webhooks/custom/{uuid}/{copr_id}/"`
		`+ url = "/webhooks/custom/{copr_id}/{uuid}/"`
		`url = url.format(uuid=token, copr_id=copr_id)`
		`if package_name:`
		`url = "{0}{1}/".format(url, package_name)`

python/copr/client/client.py

file modified

+82

		`@@ -69,6 +69,7 @@`
		`SOURCE_TYPE_PYPI = 'pypi'`
		`SOURCE_TYPE_RUBYGEMS = 'rubygems'`
		`SOURCE_TYPE_SCM = 'scm'`
		`+ SOURCE_TYPE_CUSTOM = 'custom'`

		`class CoprClient(UnicodeMixin):`
		`""" Main interface to the copr service`
		`@@ -588,6 +589,47 @@`
		`return self.process_creating_new_build(projectname, data, api_endpoint, username,`
		`chroots, background=background)`

		`+`
		`+ def create_new_build_custom(self, projectname,`
		`+ script, script_chroot=None, script_builddeps=None,`
		`+ script_resultdir=None,`
		`+ username=None, timeout=None, memory=None, chroots=None,`
		`+ background=False, progress_callback=None):`
		`+ """ Creates new build with Custom source build method.`
		`+`
		`+ :param projectname: name of Copr project (without user namespace)`
		`+ :param script: script to execute to generate sources`
		`+ :param script_chroot: [optional] what chroot to use to generate`
		`+ sources (defaults to fedora-latest-x86_64)`
		`+ :param script_builddeps: [optional] list of script's dependencies`
		`+ :param script_resultdir: [optional] where script generates results`
		`+ (relative to cwd)`
		`+ :param username: [optional] use alternative username`
		`+ :param timeout: [optional] build timeout`
		`+ :param memory: [optional] amount of required memory for build process`
		`+ :param chroots: [optional] build only with given chroots`
		`+ :param background: [optional] mark the build as a background job.`
		`+ :param progress_callback: [optional] a function that received a`
		`+ MultipartEncoderMonitor instance for each chunck of uploaded data`
		`+`
		+ :return: :py:class:`~.responses.CoprResponse` with additional fields:
		`+`
		+ - builds_list: list of :py:class:`~.responses.BuildWrapper`
		`+ """`
		`+ data = {`
		`+ "memory_reqs": memory,`
		`+ "timeout": timeout,`
		`+ "script": script,`
		`+ "chroot": script_chroot,`
		`+ "builddeps": script_builddeps,`
		`+ "resultdir": script_resultdir,`
		`+ }`
		`+`
		`+ api_endpoint = "new_build_custom"`
		`+ return self.process_creating_new_build(projectname, data, api_endpoint, username,`
		`+ chroots, background=background)`
		`+`
		`+`
		`def create_new_build_distgit(self, projectname, clone_url, branch=None, username=None,`
		`timeout=None, memory=None, chroots=None, background=False, progress_callback=None):`
		`""" Creates new build from a dist-git repository`
		`@@ -818,6 +860,46 @@`
		`})`
		`return response`

		`+ def edit_package_custom(self, package_name, projectname,`
		`+ script, script_chroot=None, script_builddeps=None,`
		`+ script_resultdir=None,`
		`+ ownername=None, webhook_rebuild=None):`
		`+`
		`+ request_url = self.get_package_edit_url(ownername, projectname,`
		`+ package_name, SOURCE_TYPE_CUSTOM)`
		`+`
		`+ data = {`
		`+ "package_name": package_name,`
		`+ "script": script,`
		`+ "builddeps": script_builddeps,`
		`+ "resultdir": script_resultdir,`
		`+ "chroot": script_chroot,`
		`+ }`
		`+ if webhook_rebuild != None:`
		`+ data['webhook_rebuild'] = 'y' if webhook_rebuild else ''`
		`+`
		`+ response = self.process_package_action(request_url, ownername, projectname, data)`
		`+ return response`
		`+`
		`+ def add_package_custom(self, package_name, projectname,`
		`+ script, script_chroot=None, script_builddeps=None,`
		`+ script_resultdir=None,`
		`+ ownername=None, webhook_rebuild=None):`
		`+`
		`+ request_url = self.get_package_add_url(ownername, projectname,`
		`+ SOURCE_TYPE_CUSTOM)`
		`+ response = self.process_package_action(request_url, ownername,`
		`+ projectname, data={`
		`+ "package_name": package_name,`
		`+ "script": script,`
		`+ "builddeps": script_builddeps,`
		`+ "resultdir": script_resultdir,`
		`+ "chroot": script_chroot,`
		`+ "webhook_rebuild": 'y' if webhook_rebuild else '',`
		`+ },`
		`+ )`
		`+ return response`
		`+`
		`def process_package_action(self, request_url, ownername, projectname, data, fetch_functor=None):`
		`if not ownername:`
		`ownername = self.username`

rpmbuild/bin/copr-sources-custom

file added

+107

		`@@ -0,0 +1,107 @@`
		`+ #! /bin/env python`
		`+`
		`+ import os`
		`+ import subprocess`
		`+ import argparse`
		`+ import logging`
		`+ import pipes`
		`+`
		`+ DEFAULT_WORKDIR = '/builddir/copr-sources-custom'`
		`+`
		`+ logging.basicConfig(level=logging.DEBUG)`
		`+ log = logging.getLogger(__name__)`
		`+`
		`+ description = """`
		`+ Generate spec + patches + tarballs using custom script in mock chroot.`
		`+ """.strip()`
		`+`
		`+ parser = argparse.ArgumentParser(`
		`+ description = description`
		`+ )`
		`+ parser.add_argument(`
		`+ "-r", "--mock-config", "--root",`
		`+ dest='config',`
		`+ required=True,`
		`+ help="mock config reference (alternative to '-r/--root' option in mock)"`
		`+ )`
		`+ parser.add_argument(`
		`+ "--script",`
		`+ required=True,`
		`+ type=argparse.FileType('r'),`
		`+ help="file to be copyied into mock chroot and executed"`
		`+ )`
		`+ parser.add_argument(`
		`+ "--hook-payload-file",`
		`+ type=argparse.FileType('r'),`
		`+ help="file with 'webhook payload content', it is going to be copied "`
		`+ "into working directory as 'hook_payload' file",`
		`+ )`
		`+ parser.add_argument(`
		`+ "--builddeps",`
		`+ help="space separated list of build dependencies (packages)"`
		`+ )`
		`+ parser.add_argument(`
		`+ "--resultdir",`
		`+ default=".",`
		`+ help="directory were the SCRIPT generates the sources within mock chroot"`
		`+ )`
		`+ parser.add_argument(`
		`+ "--workdir",`
		`+ default=DEFAULT_WORKDIR,`
		`+ help="execute the SCRIPT from with CWD=WORKDIR (within mock chroot)"`
		`+ )`
		`+`
		`+`
		`+ def run_cmd(command, **kwargs):`
		`+ log.info("running command: " + ' '.join([pipes.quote(x) for x in command]))`
		`+ return subprocess.check_call(command, **kwargs)`
		`+`
		`+`
		`+ if __name__ == "__main__":`
		`+ args = parser.parse_args()`
		`+`
		`+ user = 'mockbuild'`
		`+`
		`+ # Where the script is run from within mock chroot.`
		`+ workdir = os.path.normpath(args.workdir)`
		`+`
		`+ # Where the script's result will be copied from, by default`
		`+ # equal to workdir.`
		`+ resultdir = os.path.normpath(os.path.join(workdir, args.resultdir))`
		`+`
		`+ log.info("Working in '{0}'".format(workdir))`
		`+ log.info("Results should be created in '{0}'".format(resultdir))`
		`+`
		`+ mock = ['mock', '-r', args.config]`
		`+`
		`+ run_cmd(mock + ['--init'])`
		`+`
		`+ if args.builddeps:`
		`+ pkgs = args.builddeps.split()`
		`+ run_cmd(mock + ['--install'] + pkgs)`
		`+`
		`+ run_cmd(mock + ['--shell', 'cat - > /script'], stdin=args.script)`
		`+`
		`+ setup_cmd = 'set -ex; chmod a+x /script;'`
		`+`
		`+ prep_dir_template = ' rm -rf {d}; mkdir -p {d}; chown {user} {d};'`
		`+ setup_cmd += prep_dir_template.format(`
		`+ user=user,`
		`+ d=pipes.quote(workdir),`
		`+ )`
		`+`
		`+ run_cmd(mock + ['--chroot', setup_cmd])`
		`+`
		`+ if args.hook_payload_file:`
		`+ payload_file_inner = "{0}/hook_payload".format(pipes.quote(workdir))`
		`+ run_cmd(mock + ['--shell', 'cat - > ' + payload_file_inner],`
		`+ stdin=args.hook_payload_file)`
		`+ run_cmd(mock + ['--shell', 'chmod a+r ' + payload_file_inner])`
		`+`
		`+ cmd = 'set -xe ; cd {workdir} ; {env} /script'.format(`
		`+ workdir=pipes.quote(workdir),`
		`+ env='COPR_RESULTDIR=' + pipes.quote(resultdir),`
		`+ )`
		`+ cmd = pipes.quote(cmd)`
		`+`
		`+ run_cmd(mock + ['--chroot', 'su - {0} -c {1}'.format(user, cmd)])`

rpmbuild/copr-rpmbuild.spec

file modified

		`@@ -59,6 +59,7 @@`

		`install -d %{buildroot}%{_mandir}/man1`
		`install -p -m 644 man/copr-rpmbuild.1 %{buildroot}/%{_mandir}/man1/`
		`+ install -p -m 755 bin/copr-sources-custom %buildroot%_bindir`

		`%py3_install`

		`@@ -68,6 +69,7 @@`
		`%{python3_sitelib}/*`

		`%{_bindir}/copr-rpmbuild`
		`+ %{_bindir}/copr-sources-custom`
		`%{_mandir}/man1/copr-rpmbuild.1*`

		`%dir %attr(0775, root, mock) %{_sharedstatedir}/copr-rpmbuild`

rpmbuild/copr_rpmbuild/helpers.py

file modified

+24

		`@@ -23,6 +23,7 @@`
		`PYPI = 5`
		`RUBYGEMS = 6`
		`SCM = 8`
		`+ CUSTOM = 9`


		`def cmd_debug(result):`
		`@@ -189,3 +190,26 @@`
		`subprocess.check_output(cmd, shell=True)`
		`finally:`
		`os.chdir(cwd)`
		`+`
		`+`
		`+ def build_srpm(srcdir, destdir):`
		`+ cmd = [`
		`+ 'rpmbuild', '-bs',`
		`+ '--define', '_sourcedir ' + srcdir,`
		`+ '--define', '_rpmdir ' + srcdir,`
		`+ '--define', '_builddir ' + srcdir,`
		`+ '--define', '_specdir ' + srcdir,`
		`+ '--define', '_srcrpmdir ' + destdir,`
		`+ ]`
		`+`
		`+ specfiles = glob.glob(os.path.join(srcdir, '*.spec'))`
		`+ if len(specfiles) == 0:`
		`+ raise RuntimeError("no spec file available")`
		`+`
		`+ if len(specfiles) > 1:`
		`+ raise RuntimeError("too many specfiles: {0}".format(`
		`+ ', '.join(specfiles)`
		`+ ))`
		`+`
		`+ cmd += [specfiles[0]]`
		`+ run_cmd(cmd)`

rpmbuild/copr_rpmbuild/providers/__init__.py

file modified

		`@@ -3,6 +3,7 @@`
		`from .pypi import PyPIProvider`
		`from .spec import SpecUrlProvider`
		`from .scm import ScmProvider`
		`+ from .custom import CustomProvider`


		`__all__ = [RubyGemsProvider, PyPIProvider,`
		`@@ -17,6 +18,7 @@`
		`SourceType.RUBYGEMS: RubyGemsProvider,`
		`SourceType.PYPI: PyPIProvider,`
		`SourceType.SCM: ScmProvider,`
		`+ SourceType.CUSTOM: CustomProvider,`
		`}[source_type]`
		`except KeyError:`
		`raise RuntimeError("No provider associated with this source type")`

rpmbuild/copr_rpmbuild/providers/custom.py

file added

+94

		`@@ -0,0 +1,94 @@`
		`+ import os`
		`+ import json`
		`+ import logging`
		`+ import shutil`
		`+ import tempfile`
		`+ import subprocess`
		`+ import glob`
		`+ import requests`
		`+`
		`+ from copr_rpmbuild import helpers`
		`+ from .base import Provider`
		`+`
		`+`
		`+ log = logging.getLogger("__main__")`
		`+`
		`+`
		`+ class CustomProvider(Provider):`
		`+ chroot = 'fedora-rawhide-x86_64'`
		`+ builddeps = None`
		`+ file_script = None`
		`+ inner_resultdir = None`
		`+ inner_workdir = '/workdir'`
		`+ hook_payload_url = None`
		`+`
		`+ workdir = None`
		`+`
		`+ def __init__(self, source_json, outdir, config):`
		`+ super(CustomProvider, self).__init__(source_json, outdir, config)`
		`+`
		`+ self.outdir = outdir`
		`+ self.chroot = source_json.get('chroot')`
		`+ self.inner_resultdir = source_json.get('resultdir')`
		`+ self.builddeps = source_json.get('builddeps')`
		`+`
		`+ if 'hook_data' in source_json:`
		`+ self.hook_payload_url = "{server}/tmp/{tmp}/hook_payload".format(`
		`+ server=config.get("main", "frontend_url"),`
		`+ tmp=source_json['tmp'],`
		`+ )`
		`+`
		`+ self.workdir = outdir`
		`+ self.file_script = os.path.join(self.workdir, 'script')`
		`+ with open(self.file_script, 'w') as script:`
		`+ script.write(source_json['script'])`
		`+`
		`+`
		`+ def produce_srpm(self):`
		`+ mock_config_file = os.path.join(self.outdir, 'mock-config.cfg')`
		`+`
		`+ with open(mock_config_file, 'w') as f:`
		`+ # Enable network.`
		`+ f.write("include('/etc/mock/{0}.cfg')\n".format(self.chroot))`
		`+ f.write("config_opts['rpmbuild_networking'] = True\n")`
		`+ f.write("config_opts['use_host_resolv'] = True\n")`
		`+`
		`+ cmd = [`
		`+ 'unbuffer',`
		`+ 'copr-sources-custom',`
		`+ '--workdir', self.inner_workdir,`
		`+ '--mock-config', mock_config_file,`
		`+ '--script', self.file_script,`
		`+ ]`
		`+ if self.builddeps:`
		`+ cmd += ['--builddeps', self.builddeps]`
		`+`
		`+ if self.hook_payload_url:`
		`+ chunk_size = 1024`
		`+ hook_payload_file = os.path.join(self.outdir, 'hook_payload')`
		`+ response = requests.get(self.hook_payload_url, stream=True)`
		`+ response.raise_for_status()`
		`+`
		`+ with open(hook_payload_file, 'wb') as payload_file:`
		`+ for chunk in response.iter_content(chunk_size):`
		`+ payload_file.write(chunk)`
		`+`
		`+ cmd += ['--hook-payload-file', hook_payload_file]`
		`+`
		`+ inner_resultdir = self.inner_workdir`
		`+ if self.inner_resultdir:`
		`+ # User wishes to re-define resultdir.`
		`+ cmd += ['--resultdir', self.inner_resultdir]`
		`+ inner_resultdir = os.path.normpath(os.path.join(`
		`+ self.inner_workdir, self.inner_resultdir))`
		`+`
		`+ # prepare the sources`
		`+ helpers.run_cmd(cmd)`
		`+`
		`+ # copy the sources out into workdir`
		`+ mock = ['mock', '-r', mock_config_file]`
		`+`
		`+ srpm_srcdir = os.path.join(self.workdir, 'srcdir')`
		`+ helpers.run_cmd(mock + ['--copyout', inner_resultdir, srpm_srcdir])`
		`+ helpers.build_srpm(srpm_srcdir, self.outdir)`
		`+ shutil.rmtree(srpm_srcdir)`

praiskup commented 6 years ago

Missing things to be done:

[-] generate mock config (postponed to next PR)
[x] create the script from /get-srpm-build-task/ info provided by frontend
[x] (different semantics now) call the copr-sources-custom script (with --resultdir=$(mktemp -d))
[x] copy out the results from build chroot (mock --copy-out)
[x] run rpmbuild -bs (or something similar to get the srpm, this is long-term undesirable and redundant step since the sources could be imported to distgit without "wrapping" to src.rpm)
[x] api/cli is needed (not a blocker in the first step, where webui should be enough)
[x] webui is needed
[x] documentation (webhook payload parser example once 'generate mock config' is implemented)
[x] custom webhook + docs (PR#194)
[x] specfile fixes for installation of rpmbuild package
[x] copr-rpmbuild needs to download the hook_payload, and "export" that somehow
[x] beaker tests

clime commented 6 years ago

Thank you for this PR/suggestion. At a first glance, it seems very similar to the make_srpm method for SCM source type and I am curious about the added value here. Also note that there will raise an issue with auto-rebuilding for this Custom Source Type. With SCM, we compare clone URLs in the incoming event with what's configured for a package to know if the event relates to the package or not. That approach is not possible here (due to missing Clone URL info). The last objection is that it is unclear what should be the output of the script.

praiskup commented 6 years ago

I am curious about the added value here.

This method will work for every package out there, without ever touching existing
git repo or creating new one, just for the CI case.

.. That approach is not possible here (due to missing Clone URL info).

With this method, you need to put 'git' into minimal buildroot dependencies, and you
are responsible for cloning the repo yourselves.

The last objection is that it is unclear what should be the output of the script.

That's something which needs to be properly documented, but the output of this
script is content of %_sourcedir (tarball + spec file + maybe patches).

clime commented 6 years ago

This method will work for every package out there, without ever touching existing
git repo or creating new one, just for the CI case.

Creating a new repo just for CI case is, I think, fine.

With this method, you need to put 'git' into minimal buildroot dependencies, and you
are responsible for cloning the repo yourselves.

You probably haven't read or misunderstood my argument about auto-rebuilding.

praiskup commented 6 years ago

Creating a new repo just for CI case is, I think, fine.

I agree that it about preferences, but I prefer not to create additional repo and
not touching upstream repos or as minimally as possible.

You probably haven't read or misunderstood my argument about auto-rebuilding.

You are right. Can you elaborate? Why you need to compare clone urls?
The web-hook is not bound to particular package rebuild?

praiskup commented 6 years ago

Ah, that's this (and sort of blocker here): https://bugzilla.redhat.com/show_bug.cgi?id=1344191

praiskup commented 6 years ago

Please take another look at this WIP, there's big TODO list, but at least to understand the workflow idea, feedback needed.

rebased onto 69f36c80d3f79473f4ae965c79042b0c753c451b

6 years ago

clime commented on line 1 of rpmbuild/bin/copr-sources-custom 6 years ago

Please don't add some extra command-line tools to the copr-rpmbuild package. All the functionality needed to implement the custom method should be present in the CustomProvider below. With this "concept" of extra script, copr-backend needs to know the build type (srpm/scm/custom) to invoke the right tool. It currently does get that information but it probly won't in the future to minimize data transfer between frontend and backend. It will only get what's important to schedule/allocate the job properly (arch, owner, task_id). Also, the backend would need to parse the source_json structure, we want only builder doing that.

clime commented on line 1 of rpmbuild/bin/copr-sources-custom 6 years ago

Oh, I see you are invoking this script from the CustomProvider. I will read the whole thing first.

clime commented on line 18 of rpmbuild/main.py 6 years ago

The actual task (job definition for the srpm build) needs to be fully specified by task["source_type"] and task["source_json"]. All the required information to build the srpm should be placed there.

clime commented on line 1 of rpmbuild/copr-rpmbuild 6 years ago

Why are you creating another "local" executable. Is it possible not to do that?

clime commented on line 52 of rpmbuild/copr_rpmbuild/helpers.py 6 years ago

You shouldn't need to obtain the mock config. The frontend's chroot build config (that gets translated into the respective mock config on builders) is only relevant for rpm builds, not for srpm builds. If you want a user to be able to provide a mock configuration (like the builddeps), then this configuration should be fully specified in the custom package or build definition (unless being always the same) and also be dedicated for the srpm build itself. The rpm build afterwards should follow the rules of the other rpm builds. Why? It's according to the already existing source types and the way srpms are created for them. I think the same rules should apply for all the dynamically created srpms.

praiskup commented on line 1 of rpmbuild/copr-rpmbuild 6 years ago

I can keep this for my own debugging purposes locally, if that matters, any consequences out of curiosity? My "pros" is that it is just convenience wrapper which really simplifies testing of the software directly from git repo...

praiskup commented on line 52 of rpmbuild/copr_rpmbuild/helpers.py 6 years ago

You shouldn't need to obtain the mock config. The frontend's chroot build config (that gets translated into the respective mock config on builders) is only relevant for rpm builds, not for srpm builds.

This is somewhat crucial motivating point for me though. I need to be able to on-demand update software in chroot, and I really want to let that affect the srpm build -- e.g. if I want to devel some universal web-hook-parser library, going through official fedora repos would be too long shot. Typical usecase for me: I may well try to "fix" or develop autotools packages only in my copr repo (which are used to produce tarballs through custom method ...).

If you want a user to be able to provide a mock configuration (like the builddeps), then this configuration should be fully specified in the custom package or build definition (unless being always the same) and also be dedicated for the srpm build itself.

The 'builddeps' parameter is dedicated to srpm build only, however... I don't take that as "configuration", but rather step to do to get the srpm built (maybe instead of specifying biuld-deps, I could specify procedural "root-prepare-command").

If you speak about "additional_repos" or "additional_packages" "configuration" --
configured per-project/per-chroot, it really is mock configuration I requested globally to build against -- and thus I want to prepare the srpm against that configuration, too.

The rpm build afterwards should follow the rules of the other rpm builds. Why?
It's according to the already existing source types and the way srpms are created for them. I think the same rules should apply for all the dynamically created srpms.

I guess I am missing the point here, can you please elaborate?

praiskup commented on line 18 of rpmbuild/main.py 6 years ago

The point is that I need the copr id in question; to generate appropriate mock config.. Do you suggest to hard-code this once more into source_json?

clime commented on line 18 of rpmbuild/main.py 6 years ago

Well, there is the provider interface and you should stick to it. Otherwise, you are breaking the interface, which is bad. It's as simple as that.

praiskup commented on line 18 of rpmbuild/main.py 6 years ago

Does that mean "yes"? :) I don't feel it is clean. There's alternative option to pre-generate the mock configuration in advance -- before calling the provider.. but it would just complicate things.

praiskup commented on line 52 of rpmbuild/copr_rpmbuild/helpers.py 6 years ago

Fwiw, consider that you develop /bin/rpkg... and you want to build it in copr and check that srpm builds fine with the re-built versoin --- for this reason I think wrapping other methods into mock chroot job would be useful, too.

praiskup commented on line 18 of rpmbuild/main.py 6 years ago

Of course, we can also change the providers' interface... (is that what you meant)? Btw. that's why ask for advice since I have no significant preference...

clime commented on line 18 of rpmbuild/main.py 6 years ago

We don't want to change the provider interface. source_type and source_json fully define the srpm build and any new method should stick to it. I am afraid you will need to update your requirements on this new method.

praiskup commented on line 18 of rpmbuild/main.py 6 years ago

We don't want to change the provider interface. source_type and source_json fully define the srpm build and any new method should stick to it

Agreed. Though additional provider.prepare(task) call (usually unimplemented wouldn't be that bad).

I am afraid you will need to update your requirements on this new method.

So how? If you agree that I need that information, I'm interested to see what requirement should I update.

rebased onto 7911d8017d449353f010942b6f230f7e23283648

6 years ago

clime commented on line 18 of rpmbuild/main.py 6 years ago

The requirement that chroot (and project) settings in COPR configuration influence the srpm build. There are two good options even without that. First is to extend the make_srpm method and give it the third argument: payload. In the make_srpm script, you can do everything you need to. And note that you can have just one Git repository with srpm generation scripts for all your maintained packages. For that you can use the subdirectory setting to invoke the correct script on the builder (i.e. you set the subdirectory for a given package to point to the directory where the respective srpm generation script can be found in your repo). The second option is giving structure to your custom script with one of the sections being e.g. "additional packages".

praiskup commented on line 18 of rpmbuild/main.py 6 years ago

The requirement that chroot (and project) settings in COPR configuration influence the srpm build.

It is certainly what I want, however - same as I want to build (other) rpms against the buildroot, I want to build the srpms the same way. Please don't make me to relax this particular requirement :-/ that was something I was wishing about long time before I started doing the work and it is driving my work here :-)

I see the issue here (getting mock profile config), but nice solution will come with #185, can we agree on that? Until then, let's consider this WIP only. Btw., koji devels - after many years - now deal with the same issue (how to generate the mock config for specific target properly), while they realized that they want to generate slightly different mock config on fedora than on rhel (dnf vs yum issues).

praiskup commented on line 18 of rpmbuild/main.py 6 years ago

nice solution will come with #185,

I meant #179 (I can not edit the comments, for some reason).

clime commented on line 18 of rpmbuild/main.py 6 years ago

Sorry, but chroot and project settings are only intended for rpm building. It's already done like this for the other options to dynamically create the srpm in COPR. There are two good options to make your use-case possible so, please, stick to one them. In https://pagure.io/copr/copr/pull-request/179, you are just forcing a bad coding practice.

rebased onto a314121159a9913aaa7cb6bda35430bdc406f618

6 years ago

praiskup commented on line 18 of rpmbuild/main.py 6 years ago

Sorry, but chroot and project settings are only intended for rpm building.

Who has/had this intention? It doesn't feel correct ...

It's already done like this for the other options to dynamically create the srpm in COPR.

How this is related to the new method - we can design/define/document anything differently in the new method, can't we?

In https://pagure.io/copr/copr/pull-request/179, you are just forcing a bad coding practice.

Self-confident, IMPOV statement which makes me sad again. Let that on the random reader where the true is.... but in #179 I was trying to only find a way to de-duplicate some efforts. You know how much time we spent on finding the best way towards this... The cause for all the words here is that some code (done with good, long-time vision by me) was re-implemented instead of re-used&&fixed (this was laziness, in the better case), and left behind giving inconsistent output. Now I need to use that code once more, without re-implementing, and you say that I'm spreading a bad coding practices.

Back to the topic. I'm concentrating quite some time on the I need statements, to let you better understand (I was told that nobody can say that I don't need something, if I clearly define what my needs are.. but it seems it doesn't work!). Something more about the vision.. I need a method where:

I can do anything (turing-complete thingy) to build the sources
I don't imply 'root' powers for the scripting (e.g. for installing packages)
I don't enforce anything (toolset (e.g. /bin/make, git), srcdir layout, hook caller, etc.)
I want to be able to update the tooling used to build the sources (say experimental autotools to build the latest tar's git)
I want to do this ^^^ as if I did this on my box (first install build dependencies, and then run something "safely" - without a root access).

I don't want to loose 4. while I don't want to regress to make_srpm with 2. and 3. having 5. in mind. Does this make sense? What's do you suggest to have 4. in with good practices?

4 new commits added

[cli][python] custom method added to api
[frontend] webui: add/edit package && custom method
[rpmbuild] hack to provide proper mock config
[frontend][rpmbuild] new custom method

6 years ago

clime commented on line 18 of rpmbuild/main.py 6 years ago

Note that with make_srpm, you can do:

1) install all the packages from Makefile
2) drop root privileges
3) call an external bash script that will do the building

While in COPR, the Makefile is invoked to install the deps as well as call the script, on your local machine, you can just invoke the script directly to do the job and you don't need any special tooling except bash.

Or with the second option that I was proposing - the structured custom script with sections, you can have e.g. script that looks like this:

   chroot: "fedora-27-x86_64"
   additional_packages:
    - zsh
   additional_repos:
    - somerepo
   script_path:  "build_srpm.sh"

This snippet can be then into a mock config for a chroot in which the required command is invoked. You need to prepare some nice tool that will do this (mock config generation, chroot spawning and the script invocation) so you can say 3 is not satisfied but apart from that everything seems to be satisfied (and you will also be able to call build_srpm.sh directly on your machine).

So both options are very nice and I'll be happy if you contribute into implementation of any of those. Thank you very much!

clime commented on line 18 of rpmbuild/main.py 6 years ago

This snippet can be then translated into a mock config

praiskup commented on line 18 of rpmbuild/main.py 6 years ago

I see your point in the "declarative" way of thinking, and that make_srpm. That's fine metod, but it is completely different topic than what I'm trying to achieve. My previous comment says it all.

2 new commits added

[rpmbuild] don't build srpm against copr repo
drop the testing wrapper

6 years ago

praiskup commented 6 years ago

Web-UI preview: http://praiskup.fedorapeople.org/Screenshot_20180116_093950.png

1 new commit added

[rpmbuild] install copr-sources-custom script

6 years ago

1 new commit added

[rpmbuild] download hook payload into {workdir}

6 years ago

praiskup commented 6 years ago

If I understood @clime's POV right, using the project's mock-configuration for RPM builds and only for custom srpm method (not for other methods) would be misleading for users, while implementing it globally also for all other methods would be too complicated. So the proposed solution by @clime is additional (optional) "textarea" with additional repositories (where the project copr:// url can be specified, too) in the proposed web-form, and in the commandline API.. This sounds good to me, but please allow me to solve this point (the point 4. in my use-case/motivating description above) later after this PR, is it OK? And please correct me if I'm wrong with something.

clime commented 6 years ago

If I understood @clime's POV right, using the project's mock-configuration for RPM builds and only for custom srpm method (not for other methods) would be misleading for users, while implementing it globally also for all other methods would be too complicated. So the proposed solution by @clime is additional (optional) "textarea" with additional repositories (where the project copr:// url can be specified, too) in the proposed web-form, and in the commandline API.. This sounds good to me, but please allow me to solve this point (the point 4. in my use-case/motivating description above) later after this PR, is it OK? And please correct me if I'm wrong with something.

I guess it's completely alright.

rebased onto b30d9b705728c1cbf3e5107cbe88309a0fd4cb4c

6 years ago

praiskup commented 6 years ago

Basic documentation is added. I just noticed there's missing new cli command for custom build, are there any other major design issues so I can squash the commits and finalize?

clime commented on line 21 of doc/custom_source_method.rst 6 years ago

I don't think we need to explain to users the reasons why it is designed the way it is in the documentation. Documentation should only contain the information to make something work - not design-justifications. In other words, you can just say:

"The script is run under unprivileged user and any build requires for the script are provided as a list in an additional input field."

You seem to be comparing this method to the make_srpm method. In fact, this paragraph just sounds like "My method is better because...". I would recommend writing it in a more neutral manner.

clime commented 6 years ago

Except the one comment on documentation, looking good at a first glance. I will do a more thorough review during the day.

praiskup commented on line 21 of doc/custom_source_method.rst 6 years ago

You seem to be comparing this method to the make_srpm method. In fact, this paragraph just sounds like "My method is better because...". I would recommend writing it in a more neutral manner.

The purpose of the statement is really different. I just answered the questions we discussed before on the meeting; and I really want to make sure that this design is intentional, and convince the user that this isn't going to be changed so there's no reason to file a bug "give me a root access..".

praiskup commented 6 years ago

Except the one comment on documentation, looking good at a first glance. I will do a more thorough review during the day.

To not waste your time, let me please cleanup the PR first. I'm now interested in the first-glance issues... e.g. I haven't created much testing yet. Is there something which I have to write tests for? I was thinking about tests for rpmbuild (the new script), but that requires "mock" permissions (basically that implies root), so that's definitely not usable for %check time... :-/ thoughts?

frostyx commented 6 years ago

You seem to be comparing this method to the make_srpm method. In fact, this paragraph just sounds like "My method is better because...". I would recommend writing it in a more neutral manner.

Even if so, I would say that it's fine. Both methods have the same goal and provide a different solution. Naturally, there are advantages and disadvantages for both of them, so I think that it would be just fine to compare and motivate people to use them (both make srpm and custom) because of their advantages.

But I honestly can't see any trash talk about other methods or any attacks on them. Maybe just the "(btw. building SRPM under root is equivalently unfortunate idea as building RPMs under root)." which I believe was not intended as bashing make_srpm, but rather a general statement.

clime commented 6 years ago

You seem to be comparing this method to the make_srpm method. In fact, this paragraph just sounds like "My method is better because...". I would recommend writing it in a more neutral manner.

Even if so, I would say that it's fine. Both methods have the same goal and provide a different solution. Naturally, there are advantages and disadvantages for both of them, so I think that it would be just fine to compare and motivate people to use them (both make srpm and custom) because of their advantages.
But I honestly can't see any trash talk about other methods or any attacks on them. Maybe just the "(btw. building SRPM under root is equivalently unfortunate idea as building RPMs under root)." which I believe was not intended as bashing make_srpm, but rather a general statement.

The docs should contain description needed to make something work for a user. Anything else is then just confusing.

clime commented on line 6 of frontend/coprs_frontend/coprs/templates/coprs/detail/add_build/custom.html 6 years ago

looks like there is copy-paste bug here.

praiskup commented 6 years ago

But I honestly can't see any trash talk about other methods or any attacks on them. Maybe just the "(btw. building SRPM under root is equivalently unfortunate idea as building RPMs under root)." which I believe was not intended as bashing make_srpm, but rather a general statement.

I would bet that from the outside this doesn't feel non-neutral or attacking the make_srpm method -- but I agree that we are able to compare, so I'll remove that. Originally when writing this, I had the %(bin sh) macros in mind (and the reasons for srpm building movement from dist-git to builders), rather than make_srpm method.

clime commented on line 68 of doc/custom_source_method.rst 6 years ago

I don't think you can currently use the other /webhooks/github/ and /webhooks/gitlab/ entry points with the custom source type because of PackagesLogic.get_for_webhook_rebuild "filtering" (which is done by scanning source_json for a particular clone url).

clime commented on line 9 of frontend/coprs_frontend/coprs/views/coprs_ns/coprs_builds.py 6 years ago

Looks like another copy-paste bug here. Also below.

clime commented 6 years ago

I tried both of the examples: trivial and simple and none of them work, which is kind of funny given the names. Apart from providing working examples, please, provide beaker tests.

I have some additional minor notes to the code but given the reload time after each comment, I think I will save them for now.

Please, don't forget to implement copr buildcustom subcommand (and yes, it should have this stupid name).

3 new commits added

[frontend] custom form - more descriptive fields
[frontend] drop c&p error
[doc] rephrase custom source method docs

6 years ago

rebased onto 2eb50dca4c64159398c5835640ac8e853072effd

6 years ago

2 new commits added

[cli][doc][frontend][python][rpmbuild] new custom source method
[frontend] swap arguments in custom webhook and document

6 years ago

2 new commits added

[cli][doc][frontend][python][rpmbuild] new custom source method
[frontend] swap arguments in custom webhook and document

6 years ago

praiskup commented 6 years ago

copy paste bugs should be OK now
the copr buildcustom command added
web UI now has Build form for Custom method, too
how the build form looks like now (@msuchy please take a look) https://praiskup.fedorapeople.org/Screenshot_20180124_184936.png
the examples are now fixed, should be working
I now explicitly documented that only custom webhook works for custom source method

Please have a look. I need to have a look at the beaker-tests now ...

2 new commits added

[cli][doc][frontend][python][rpmbuild] new custom source method
[frontend] swap arguments in custom webhook and document

6 years ago

clime commented 6 years ago

how the build form looks like now ( @msuchy please take a look) https://praiskup.fedorapeople.org/Screenshot_20180124_184936.png

The text in the script area is very difficult to read and understand. What if I want to put there just...

 #!/bin/sh
 pyp2rpm -t epel6 motionpaint > motionpaint.spec

I would prefer only something really simple there, like one line saying:

# write a shell script that generates spec and sources...

That line says what's expected much more clearly.

That info below 'Result Directory' field:

 default= . ; the directory is resolved relatively to script's current working directory

That's just really cumbersome description. Especially the default=.; part. Also...shouldn't it be just saying:

(i) path relative to the script location

And into the field itself, you should add "Optional - " as we do it for other methods. You don't even need to say that default is .. That just comes naturally. I don't want to be figuring out descriptions for you but there is too much information there. Please, make the interface nicer.

You don't need to say everywhere that it's "mock chroot" where the script is evaluated. Nobody cares about it. It should be say just "chroot", not "mock chroot". You are unnecessarily tying it down to a particular implementation that we use today (and probably will be using for some time). It could also be evaluated in a systemd container or dedicated virtual machine and it shouldn't matter. This is quite important point. Chroot in COPR simply means os-version-arch triplet. We could also use word "target". How we implement building for a given "target" is our thing. E.g. here:

(i) packages to be preinstalled into mock chroot before script is run

"into mock chroot" part is completely redundant there. You could just say

(i) packages to be preinstalled before the script is run

(i) those packages will be installed into the script's environment before its execution

or even better:

(i) packages that the script requires for its execution

The problem with this approach for describing things is that is that if one thing changes, lots of other things need to change as well. That's not good.

I don't know why you changed "fedora-latest-x86_64" to "The latest branch Fedora (x86_64)". That's much worse especially because in the info line below, you again describe it as "fedora-latest-x86_64". I would just stick to "fedora-latest-x86_64" and live with it.

That info below the script text-area:

(i) script to generate sources used to generate source RPM ...

That's not a good description.

I know this was intended to be checked by msuchy but I really need to comment on this as well. Before those changes were made, the interface was actually much easier to understand. I thought you would only add a single info line below the 'Chroot' select field describing what "fedora-latest" is. That would be sufficient. Please, just trim the descriptions down as much as you can. There is simply too much information.

clime commented on line 7 of cli/copr_cli/main.py 6 years ago

buildgem? Could you please check for copy-paste errors again? Don't use copy-paste if you don't change what needs to be changed.

clime commented on line 74 of cli/copr_cli/main.py 6 years ago

it's "buildcustom" command, not "custombuild" command.

clime commented on line 21 of doc/custom_source_method.rst 6 years ago

Can you, please, drop any design justifications from the method description? This whole paragraph basically.

clime commented on line 51 of doc/custom_source_method.rst 6 years ago

resultdir ... where the script generates its output. By default, it is assumed to be current working directory of the script.

(Note that for this, you first need to specify, what's the current working directory for the script in the above description. Otherwise, you should say "By default, they are assumed to be generated to the same directory where the script is located")

clime commented on line 52 of doc/custom_source_method.rst 6 years ago

Additional note: Any mention of "dist-git" is completely redundant and inappropriate here.

clime commented on line 75 of doc/custom_source_method.rst 6 years ago

You probably meant owner/repo.git there. Otherwise, this description is very nice.

clime commented on line 82 of doc/custom_source_method.rst 6 years ago

Very good job in this part.

clime commented on line 9 of doc/user_documentation.rst 6 years ago

I would be careful and just say:

This source type uses a user-defined script to generate sources (which are later used to create SRPM). This is again completely redundant mention of copr-dist-git.

clime commented on line 5 of frontend/coprs_frontend/coprs/models.py 6 years ago

source_json_fixup. That's not a good name. Not for method and not even for its arguments.

It should be:

postprocess_source_json(source_json, source_type).

Also actually, this method should exist at all. The whole content should be in the Build's constructor because you are not going to need to call this code from anywhere else. And the if not 'chroot' in source_dict...that value for chroot should be generated by input form for the custom method, not here.

clime commented on line 16 of rpmbuild/copr_rpmbuild/helpers.py 6 years ago

You could invoke just rpkg srpm there which does all those macro settings.

clime commented on line 56 of rpmbuild/bin/copr-sources-custom 6 years ago

log_cmd method that does invoke subprocess....

clime commented on line 32 of rpmbuild/copr_rpmbuild/providers/custom.py 6 years ago

soo. self.chroot attribute exists in the object only if 'chroot' in source_json...That's not correct way code in python. Especially, given that you use self.chroot later in produce_srpm.

Please, rather use:

self.chroot = source._json.get('chroot')

That also applies to the lines below.

clime commented on line 15 of python/copr/client/client.py 6 years ago

The variable (and argument) naming should be the same for build and also the package methods.

clime commented 6 years ago

I generally like this but there are many minor things that should be fixed. Also, I am not sure why the script is supposed to produce sources and not srpm directly. It would simplify description at lot of places but okay, I guess, it's a valid option. Anyway, thanks for you work here. I can already see now that it's becoming useful (and it even doesn't exist yet).

clime commented 6 years ago

I just noticed that the srpm is created outside of the chroot. Is that really what we want? What if a user wants to pass a certain --define to rpmbuild (before I was imagining that he/she would do it by creating ~/.rpmmacros with the desired macros but this is not possible).

praiskup commented on line 21 of doc/custom_source_method.rst 6 years ago

hm, -1, but if that's the blocker ...

praiskup commented on line 16 of rpmbuild/copr_rpmbuild/helpers.py 6 years ago

-1, rpkg is not what I need (it does too much, I want low level tool if any), and this ideally should go away entirely (there are no technical reasons to even create source rpm).

praiskup commented on line 32 of rpmbuild/copr_rpmbuild/providers/custom.py 6 years ago

I know you like writing, but saying "source_json.get(key) is cleaner" would be much firendly ;)

rebased onto b9b378ca9b0be54e6ac01f2ab73fe3480715a4ad

6 years ago

praiskup commented 6 years ago

Thanks a lot the review. Looks really better now: http://praiskup.fedorapeople.org/Screenshot_20180125_174013.png

praiskup commented 6 years ago

The last version: http://praiskup.fedorapeople.org/custom-method-latest.png

3 new commits added

review fixes
[cli][doc][frontend][python][rpmbuild] new custom source method
[frontend] swap arguments in custom webhook and document

6 years ago

clime commented 6 years ago

As for srpm vs. raw source generation...The problem is that we are generating srpms everywhere else and it's unlikely to go away. It would just really make things easier to explain. And you would also allow people to alter the srpm building process itself, which is something they might want to do (i.e. they will experiment with the build script by always changing just one --define value in the rpmbuild command line, then submitting build, changing again, then submitting build, etc.). I would like to discuss this again because I think you are thinking way too big here.

Edited 6 years ago by clime

clime commented 6 years ago

http://praiskup.fedorapeople.org/custom-method-latest.png

Yes, this is very good. I would go back to "fedora-latest-x86_64" in the select field or won't be using the word "branched" but "stable" or "released" instead (because lots of people probably won't have an idea why it is called "branched") but otherwise nice.

clime commented on line 44 of frontend/coprs_frontend/coprs/forms.py 6 years ago

What about the latest "branched" Fedora for other archs? You should cover all the archs here.

clime commented on line 16 of rpmbuild/copr_rpmbuild/helpers.py 6 years ago

Well, rpkg actually does exactly what you are doing here if all the sources are already present (which is the case here).

clime commented 6 years ago

It looks good. You haven't dropped that "why this design" paragraph that really shouldn't be present in a user documentation but I don't mind it that too much given that this PR is now in a pretty good shape. But really...if it's good, then you don't need to explain why it is good. Anyway, thanks. I am now looking forward to see the tests (I need to additionally test out the examples).

praiskup commented 6 years ago

It looks good. You haven't dropped that "why this design" paragraph that really shouldn't be present in a user documentation but I don't mind it that too much given that this PR is now in a pretty good shape. But really...if it's good, then you don't need to explain why it is good.

It is not about describing that it is good ... it is really "why"...

As for srpm vs. raw source generation...The problem is that we are generating srpms everywhere else and it's unlikely to go away. It would just really make things easier to explain.

... e.g. here, if you require that user has to do the 'rpmbuild -bs', that script is not anymore (easily) portable to Debian, Gentoo, etc. (described in the "why" docs). Anyway, script-API is different from metdhod-API (and the method genertes SRPM, so it doesn't diverge).

That reminds me -- I didn't touch your hint aboout this before; yes generating the srpm on host is desired, because it is more likely that srpm generated on host will be "importable" on dist-git machine (SRPM generated in fedora-rawhide-x86_64 can have too new feature to import correctly; epel-6-x86_64 can be theoretically too old).

Well, rpkg actually does exactly what you are doing here if all the sources are already present (which is the case here).

At least I now learned that the rpm is configured by .rpmmacros, which is (at least for every use-case I ever had in mind) unexpected side-effect... What if we created "tarball" instead of src.rpm, in the end?

praiskup commented 6 years ago

As for srpm vs. raw source generation...The problem is that we are generating srpms everywhere else and it's unlikely to go away. It would just really make things easier to explain. And you would also allow people to alter the srpm building process itself, which is something they might want to do (i.e. they will experiment with the build script by always changing just one --define value in the rpmbuild command line, then submitting build, changing again, then submitting build, etc.). I would like to discuss this again because I think you are thinking way too big here.

If the SRPM is generated with some --define, it is anyways extracted on dist-git side, and imported... We really use the SRPM as transport format, and that's huge benefit of copr which would disappear with dist-git machine was not used as is. Now, once imported, users are sure that they bulid byte-by-byte the same content in all chroots...

praiskup commented 6 years ago

Yes, this is very good. I would go back to "fedora-latest-x86_64" in the select field or won't be using the word "branched" but "stable" or "released" instead (because lots of people probably won't have an idea why it is called "branched") but otherwise nice.

I'll take a look at this, ok. The "branched" is really correct word in fedora world..., but latest is fine, too.

I'm not convinced that other arches are needed, should I add them now, or can we wait for explicit request?

clime commented 6 years ago

If the SRPM is generated with some --define, it is anyways extracted on dist-git side, and imported... We really use the SRPM as transport format, and that's huge benefit of copr which would disappear with dist-git machine was not used as is. Now, once imported, users are sure that they bulid byte-by-byte the same content in all chroots...

Except that the content of external repositories might be completely different. You are overestimating the value of the feature. But even if that was true, there is another way to assure it (to keep the built srpms, which we already do). But I don't want to go into this now. At the moment, copr-dist-git has still its value at the moment.

clime commented 6 years ago

Except that the content of external repositories might be completely different. You are overestimating the value of the feature. But even if that was true, there is another way to assure it (to keep the built srpms, which we already do). But I don't want to go into this now. At the moment, copr-dist-git has still its value at the moment.

Actually....we could just send every srpm (even uploaded ones) to copr-backend and store it there and then use srpm_url attribute for rebuilding. This is actually the good way to do things :).

clime commented 6 years ago

Except that the content of external repositories might be completely different. You are overestimating the value of the feature. But even if that was true, there is another way to assure it (to keep the built srpms, which we already do). But I don't want to go into this now. At the moment, copr-dist-git has still its value at the moment.

Actually....we could just send every srpm (even uploaded ones) to copr-backend and store it there and then use srpm_url attribute for rebuilding. This is actually the good way to do things :).

We can drop the copr-dist-git redundancy and use it to deploy our own writeable DistGit! \o/

clime commented 6 years ago

Yes, this is very good. I would go back to "fedora-latest-x86_64" in the select field or won't be using the word "branched" but "stable" or "released" instead (because lots of people probably won't have an idea why it is called "branched") but otherwise nice.

I'll take a look at this, ok. The "branched" is really correct word in fedora world..., but latest is fine, too.
I'm not convinced that other arches are needed, should I add them now, or can we wait for explicit request?

Please, add them now. There is no reason to "prioritize" one arch over another.

1 new commit added

[frontend] add fedora-latest-* for other arches

6 years ago

praiskup commented 6 years ago

Actually....we could just send every srpm (even uploaded ones) to copr-backend and store it there and then use srpm_url attribute for rebuilding. This is actually the good way to do things :).

I consider the actual dist-git very useful as I browse cgit html pages all the time (mostly internally, Fedora copr has issues with cgit most of the time unfortunately). It is invaluable to see differences in specfiles between two successful imports (though yes, I wouldn't be against cleaning-up the lookaside cache).

We can drop the copr-dist-git redundancy and use it to deploy our own writeable DistGit! \o/

This is orthogonal feature, automatic imports can coexist with manual pushes; same as this is supported by fedpkg workflows. Off topic, sorry :-)

Except that the content of external repositories might be completely different.
You are overestimating the value of the feature.

I don't understand this, the content of imported dist-git should be completely the same, that's the point. But yes, that can be achieved different ways.... but...

But even if that was true, there is another way to assure it (to keep the built srpms, which we already do). But I don't want to go into this now. At the moment, copr-dist-git has still its value at the moment.

... this is possible, but note that srpms are not portable. If you build the srpm on f27 (currently on builders), you can not be 100% sure it is importable in epel-5-x86_64 (some copr instances might still support this, I have a request to support rhel-4-x86_64 chroot actually). Anyways, I know that last time we discussed this the conclusion was to go through survey first .. about "how much automatic imports collide with manual pushes".

Edited 6 years ago by praiskup

clime commented 6 years ago

I consider the actual dist-git very useful as I browse cgit html pages all the time (mostly internally, Fedora copr has issues with cgit most of the time unfortunately). It is invaluable to see differences in specfiles between two successful imports (though yes, I wouldn't be against cleaning-up the lookaside cache).

I believe we will be able (if required) to provide the means to maintain the dist-git imports by providing post-build hooks so that you can plug-in the distgit import mechanism into copr-frontend after a srpm build is completed. It's likely to disappear from the core, however, so that we can utilize the freed up resources in Fedora (the copr-dist-git machine) for something else (writeable DistGit). The generated srpms will be kept on backend and pruned by prunerepo after some period of time. The colored diffs on specs that the cgit provides are nice but it's not a reason to keep copr-dist-git as a required feature. Seems rather like an optional thing right now. So...it's likely to become an optional feature, possibly implemented by a plugin. In this case, we will also need to rework copr-dist-git to react to post events (instead of polling copr-frontend periodically) to execute the import.

This is orthogonal feature, automatic imports can coexist with manual pushes; same as this is supported by fedpkg workflows. Off topic, sorry :-)

I am not sure how fedpkg supports that. Anyway, this is rather a server-related (not client-related) problem and I haven't seen any automatically imported packages on src.fedoraproject.org. Also it's not really orthogonal for me because we will be able to re-use the copr-dist-git machine for something much more useful. The current solution is not very economical.

Except that the content of external repositories might be completely different.
You are overestimating the value of the feature.

I don't understand this, the content of imported dist-git should be completely the same, that's the point. But yes, that can be achieved different ways.... but...

I was just saying that it's nice that you have the same sources (the imported content) but during the rpm build the versions of downloaded build dependencies might be different. Anyway, we will be keeping the built srpms, just probably not in their extracted form.

... this is possible, but note that srpms are not portable. If you build the srpm on f27 (currently on builders), you can not be 100% sure it is importable in epel-5-x86_64 (some copr instances might still support this, I have a request to support rhel-4-x86_64 chroot actually). Anyways, I know that last time we discussed this the conclusion was to go through survey first .. about "how much automatic imports collide with manual pushes".

I don't exactly see what you see with the importability problem of the built srpms. I believe that's practically a non-existent issue because the srpm binary format itself would need to be different across distro versions for this problem to occur. But...even if that was the case...why is it important that our srpms cannot be imported on a rhel-4 machine? I don't see it. I mean, I can see some kind of possible issues with this but I am not sure how practical they really are.

Or if you mean that the f27 srpms cannot be used for a subsequent rpm build in rhel-4 chroot, that I understand. Does it actually happen? if it is the case, we can first extract the stored srpm on host directly and then use the --buildsrpm feature of mock to rebuild the srpm for the given chroot so that it is compatible.

And yes, there was some survey. It doesn't need to conflict but it's also not a necessary feature to have on copr-frontend. It's not necessary because we can provide tooling in rpkg that will make the imports easy-to-do on command-line (basically it will be rpkg import <srpm_url>...maybe it's possible even now, I need to have a look. It will be on-demand importing. We can discuss it on next meeting. I am open to suggestions!

praiskup commented 6 years ago

Or if you mean that the f27 srpms cannot be used for a subsequent rpm build in rhel-4 chroot, that I understand. Does it actually happen?

That's what I meant, yes. It is nitpicking though, I don't object against having srpm in place as it is now -- I'm just saying that if we sent extracted srpms (or rather tarballs) from bulider to backend/dist-git, that would be much cleaner. The custom method generates srpm, so I hope that is fine...

And yes, there was some survey.

Do you have the link to the discussion?

Btw., thanks for all the ideas, but I would rather went back to the PR issue, we can discuss this later.

clime commented 6 years ago

I am ok with srpms.

Do you have the link to the discussion?

Nope, It was really just ear-to-ear info.

Btw., thanks for all the ideas, but I would rather went back to the PR issue, we can discuss this later.

Yup, sure. Waiting for the tests now ;).

rebased onto ec98b983f9639f5b347ad3cad61027cdf2d90284

6 years ago

7 new commits added

[beaker-tests] test the new custom method
[rpmbuild] chrooted command names has $PWD/hook_payload
[rpmbuild] custom: raise for invalid spec file
[frontend] add fedora-latest-* for other arches
review fixes
[cli][doc][frontend][python][rpmbuild] new custom source method
[frontend] swap arguments in custom webhook and document

6 years ago

1 new commit added

[rpmbuild] custom method: always initialize

6 years ago

praiskup commented 6 years ago

PTAL

3 new commits added

[beaker-tests] test the new custom method
[cli][doc][frontend][python][rpmbuild] new custom source method
[frontend] swap arguments in custom webhook and document

6 years ago

clime commented on line 1 of beaker-tests/Regression/builder/tests/custom-method/run.sh 6 years ago

What was the reason to make new directory for 'test' builder? Also this doesn't really test builder only...if there are things like copr edit-package.

praiskup commented on line 1 of beaker-tests/Regression/builder/tests/custom-method/run.sh 6 years ago

I originally thought that I'll provide spec file directly...

rebased onto c18027af9a79ea9637ffb4596ebfdafc882912c1

6 years ago

praiskup commented 6 years ago

What was the reason to make new directory for 'test' builder?

Fixed.

Also this doesn't really test builder only...if there are things like copr edit-package.

The primary goal is to test things on builder. The side-effect is that it tests the cli and
frontend..

clime commented 6 years ago

What was the reason to make new directory for 'test' builder?
Fixed.

I don't know what you have fixed but anyway...you shouldn't be spawning frontend and call methods against it in builder Regression tests. Those tests should only contain invocations of copr-rpmbuild with the desired input to be tested. You can use ~/copr/mocks to supply the input to copr-rpmbuild.

The stuff like copr edit-package-custom or copr build-package belongs to beaker-tests/Sanity. These tests that are running against dev instances by default but they can be configured to use e.g. the local docker stack.

Problematic things (summary):
- Vagrant is deprecated
- copr-mocks should be used to test copr-rpmbuild instead of spawning full frontend (related to this point, there is lots boiler-plate code that's not really needed)

praiskup commented 6 years ago

I don't know what you have fixed but anyway...you shouldn't be spawning frontend and call methods against it in builder Regression tests.

Because?

Vagrant is deprecated

Other Regression tests are using it (frontend). What obsoleted Vagrant? Where this is stated...

copr-mocks should be used to test copr-rpmbuild instead of spawning full frontend (related to this point, there is lots boiler-plate code that's not really needed)

There's nothing more unsympathetic than copr-mocks to me :-(. What's the benefit in writing some code which can easily get de-synchronized with the real code of frontend? Are you able to help here with the fix (patch)? I agree with the boiler-plate code POV, but the design is unfortunate.. One should be able to specify what version of package I want to test (git latest version, stable version, released (fedora) version) plus what versions of particular components to test against (frontend, git/stable/released ...) and that's it.

clime commented 6 years ago

I don't know what you have fixed but anyway...you shouldn't be spawning frontend and call methods against it in builder Regression tests.

Because?

Because it's very inefficient and you don't need to do it. copr-mocks will do the necessary job. Those tests will should be run automatically at some point and we cannot just spawn and provision some machine when it's not needed.

Vagrant is deprecated

Other Regression tests are using it (frontend). What obsoleted Vagrant? Where this is stated...

We now have the docker stack and I believe that's better maintained.

copr-mocks should be used to test copr-rpmbuild instead of spawning full frontend (related to this point, there is lots boiler-plate code that's not really needed)

There's nothing more unsympathetic than copr-mocks to me :-(. What's the benefit in writing some code which can easily get de-synchronized with the real code of frontend? Are you able to help here with the fix (patch)? I agree with the boiler-plate code POV, but the design is unfortunate.. One should be able to specify what version of package I want to test (git latest version, stable version, released (fedora) version) plus what versions of particular components to test against (frontend, git/stable/released ...) and that's it.

copr-mocks has perfectly fine design for its job (testing separatelly copr-dist-git and copr-backend, and now copr builder as it looks. It provides the frontend's /backend/ intefaces with just minimal implementation. Test tasks are loaded from a file and published at the respective interfaces and fetched by the other services that are being tested. Sure if you want some help, let me know. /backend/get-srpm-build-task doesn't seem to be implemented yet in copr-mocks. Feel free to implement it or let me know if I should do it.

Edited 6 years ago by clime

praiskup commented 6 years ago

I would suggest you to drop Vagrant from the testsuite, then. Even though I am
not huge fan of docker, waiting for cri-o and friends... Before done so, it
sounds like I have to go and debug why the docker-stack doesn't work for me (it
failed for me last time after 20minutes of building something). Can we solve
this later?

Beaker tests are unfortunately not about performance (zero paralelism), and I
see zero additional value in testing of the components separately. More, I see
a lost opportunity to test more parts of the real code.

Sure if you want some help, let me know.

How much work would you spent on editing the proposed tests to work according to
your ideas with copr-mocks? Because I would really appreciate the help here...
(I'm not fan of the copr-mocks stuff, and hardly will be in the future). I'm
not really sure that you'll get to have some significant speedup, yes the
frontend build time... but otherwise.

Please consider my offer with help (not a prerequisite for this PR, though) with
optimizing the tests so they work also without copr-mocks. So we can test real
components against real components. By that I mean -- we should really make the
deployment of copr stack a matter of seconds, e.g. in docker for now. E.g. by
building copr-{frontend,backend,dist-git} docker images, and pushing them to
fedora registry on a regular basis... so, if that's enough, people don't have
to waste the time with building the images.

clime commented 6 years ago

Beaker tests are unfortunately not about performance (zero paralelism)

Not sure what you mean. I run those tests during production deployments and I certainly don't want to wait 30 mins until something gets provisioned that is not required in the tests (because it can be replaced by copr-mocks that are started instantly).

More, I see a lost opportunity to test more parts of the real code.

In "builder" regression tests only the builder should be tested not to get obfuscated results.

How much work would you spent on editing the proposed tests to work according to
your ideas with copr-mocks? Because I would really appreciate the help here...

Well, I offered you to implement get-srpm-build-task into copr-mocks. I didn't say I will be doing all the other changes for you. I will be on IRC next week, so please, feel free to ping me with questions.

Please consider my offer with help (not a prerequisite for this PR, though) with
optimizing the tests so they work also without copr-mocks. So we can test real
components against real components. By that I mean -- we should really make the
deployment of copr stack a matter of seconds, e.g. in docker for now. E.g. by
building copr-{frontend,backend,dist-git} docker images, and pushing them to
fedora registry on a regular basis... so, if that's enough, people don't have
to waste the time with building the images.

Ye, please, let's focus on finishing this PR with the current test infrastructure that uses copr-mocks.

clime commented 6 years ago

If it can wait we also do a quick team coding session at the office where I can give you more specific how-to on the required changes. It shouldn't be that hard but in case you are unclear about them, then this could help as well.

praiskup commented 6 years ago

Not sure what you mean. I run those tests during production deployments and I certainly don't want to wait 30 mins until something gets provisioned that is not required in the tests (because it can be replaced by copr-mocks that are started instantly).

Good question is why it takes 30 minutes to provision. We should get the provision
matter of seconds or at most few minutes (openshift-like speed).

Well, I offered you to implement get-srpm-build-task into copr-mocks. I didn't say I will be doing all the other changes for you. I will be on IRC next week, so please, feel free to ping me with questions.

copr-mocks is not a something I'll ever use, and it is against my belief.

praiskup commented 6 years ago

In "builder" regression tests only the builder should be tested not to get obfuscated results.

This opinion deserves highlight. So if the test for rpmbuild finds an issue in cli or frontend, it is obfuscating... :-) No, taking care of copr-mocks is something I don't want to waste time on.
It just delays solution of real problems (slow startup time).

frostyx commented 6 years ago

Before done so, it sounds like I have to go and debug why the docker-stack doesn't work for me (it failed for me last time after 20minutes of building something).

@praiskup, there is a known issue of frontend build failing because of docs generation. It was fixed in 8f7b53b5. Hopefully, it will help.

praiskup commented 6 years ago

@frostyx thanks for the help, but hacking on copr-mock is like writing emulator for something which we can trivially use without emulation. Feel free to work on that, but I wont do that.

I see no major issue with the tests in this PR, but feel free to merge only the first two patches (without beaker tests).

Edited 6 years ago by praiskup

clime commented 6 years ago

@frostyx thanks for the help, but hacking on copr-mock is like writing emulator for something which we can trivially use without emulation. Feel free to work on that, but I wont do that.
I see no major issue with the tests in this PR, but feel free to merge only the first two patches (without beaker tests).

This PR is unfinished because satisfying tests were not yet provided :( and it is, therefore, unsuitable for merging.

clime commented 6 years ago

Here are the options that you have to fix the PR:

1) keep only basic set of Sanity (integration) tests (that will go into beaker-tests/Sanity directory) that create/edit/build custom packages through copr-cli interface and that also test the copr-cli buildcustom command. Take e.g. look at beaker-tests/Sanity/copr-cli-basic-operations/upload_authentication.sh. So if you deliver just beaker-tests/Sanity/copr-cli-basic-operations/custom.sh (something that can be easily run and tested) and nothing else then it's also good.

2) Fix the builder tests (beaker-tests/Regression/builder) so that they don't spawn any unnecessary copr machines. The regression tests are always focused to test just one particular COPR component in separatation and they should do nothing else than that (we have integration/Sanity tests to test COPR stack as a whole). We have copr-mocks package to read build tasks from a file and supply them to other components as copr-frontned would normally do. These tasks, read from a file, represent the test data set. You can use this package or you can also implement what was described in https://pagure.io/copr/copr/issue/102 and test copr-rpmbuild directly from command-line without the need for external http interface providing the tasks.

I am ok with any of those two options and the first one should be quite easy. You can also implement both of them if you feel like it. Thanks for your work so far. I hope we can get this to the finishing line.

praiskup commented 6 years ago

I must say that the division of tests to two categories -- Sanity and Regression -- is wrong, and I don't think that's how the beakerlib is normally used (try rhpkg tests <PACKAGE> internally).
We should be able to run both kinds of tests against both CI provisioned copr stack and development/production stack. So 1) sounds like you want me to write something I'm not able to check that it works easily, and 2) is no-go with copr-mocks (reasons said before). I can add more tests related to copr-sources-custom as suggested in #102. But that would mean that I would have to drop all the coverage I wasted a lot of time on.

This hostile environment makes me tired, and I'm running out of energy. So if you wan't to drop the tests (you claim them bring just obfuscation), please do it yourself. I'm not going to rewrite tests against docker, because 'vagrant up' is used on two more places in the beaker-tests directory -- and forcing me to not use it is your random whim. I really think that the tests I spent time on bring something good, and thus I'm taking the 3) option -- waiting for your help with beaker-tests. Plus, count with my help I mentioned before (solving the slow-start problem).

clime commented 6 years ago

1) sounds like you want me to write something I'm not able to check that it works easily

It's quite easy to check it. Spawn docker instance. Then you need to update ~/.config/copr in your testing environment to point to the docker intance. And you should be done. You can now test against docker instance.

2) is no-go with copr-mocks (reasons said before).

Ye, none of the reasons I really accept. Regression tests were made to tests components in isolation on their published interfaces. copr-mocks allows it and it also allows to formally specify tasks to be run. It's a good and useful tool.

I won't be persuading you to fix the PR. I politely asked you several times to fix the problems in the PR and you haven't done it regrettably. I am not sure why I should be finishing the PR for you. If you ping me on IRC or personally I can help you if you have any troubles.

frostyx commented 6 years ago

I've looked at the tests and decided to solve the issue by re-writing them as Sanity tests. Can you guys check the patch, whether it is alright?
https://frostyx.fedorapeople.org/pagure/0001-beaker-tests-sanity-test-custom-method.patch

Then I would like to ask you @praiskup to rebase the branch against the master. I did it myself, but kinda sloppy. Also, I didn't remove the original tests.

praiskup commented 6 years ago

Thank you @frostyx, looks fine. I'm OK to rebase now, should I @clime? (removing of the
tests I added, and keep only jakub's tests (under his name).