Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARROW-2676: [Packaging] Deploy build artifacts to github releases #2109

Closed
wants to merge 35 commits into from

Conversation

kszucs
Copy link
Member

@kszucs kszucs commented Jun 6, 2018

Added a new task which trigger crossbow builds on master@crossbow.
See travis output https://travis-ci.org/kszucs/crossbow/builds/388667590

Here are the boxes we need to check:

  • Create a separate tagged branch that contains a YAML file indicating the information about each task created as part of the run. So there should be one entry for each job that was created -- the git hash for the task, the CI service used to run the task, etc. It should also indicate if one or more artifacts are expected to be uploaded

  • Write a status tool which can query the status of a particular run and determine if the run is complete (needs cleanup)

  • Can we run each desired task in a particular CI service

  • We can determine the list of created tasks associated with a particular run

  • Tasks should be configured with the tag name, and artifacts should be uploaded to GitHub under the tag which should appear as a release on the repo

  • Each task can upload its artifacts to a deterministic central location (e.g. GitHub), where the artifacts are not commingled with any other run
    -> only linux packages are failing, I suggest resolving it in a subsequent PR (issue https://issues.apache.org/jira/browse/ARROW-2713)

  • We can determine whether all the expected artifacts from a particular run have been successfully uploaded (i.e. to GitHub) to be done in https://issues.apache.org/jira/browse/ARROW-2724

  • We can download all the artifacts from a successful run and GPG sign them for purposes of a release vote

Example of artifacts available here https://github.com/kszucs/crossbow/releases
Jobs and tasks here https://github.com/kszucs/crossbow/branches
Job definition here https://github.com/kszucs/crossbow/blob/build-36/job.yml

@kszucs kszucs force-pushed the nightly branch 2 times, most recently from c3a68f1 to dd747c2 Compare June 10, 2018 14:46
@codecov-io
Copy link

codecov-io commented Jun 10, 2018

Codecov Report

Merging #2109 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2109      +/-   ##
==========================================
- Coverage   86.39%   86.39%   -0.01%     
==========================================
  Files         230      230              
  Lines       40706    40706              
==========================================
- Hits        35167    35166       -1     
- Misses       5539     5540       +1
Impacted Files Coverage Δ
cpp/src/arrow/util/thread-pool-test.cc 98.91% <0%> (-0.55%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ef9888...87860d0. Read the comment docs.

wheel-linux \
wheel-win \
wheel-osx \
linux-packages
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about this. I was thinking we would want to schedule nightlies using a cronjob of some kind. Or at least the manifest of nightly jobs would be specified in a YAML file someplace

Copy link
Member Author

@kszucs kszucs Jun 12, 2018

Choose a reason for hiding this comment

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

This would be just for convenience, to use travis' cron instead of a self hosted one.
BTW I'm just refactoring the branching logic to support tagging / deploying (github releases).

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a fine place to keep the nightly jobs list for now. There's nothing preventing us from moving this to a non-travis cron in the future if there's some reason we would need to do that.

# branch: defaults to name
# platform: osx|linux|win
# template: path of jinja2 templated yml
# params: optional extra parameters
Copy link
Member

Choose a reason for hiding this comment

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

Having the template be a .travis.yml or .appveyor.yml file only in need of template processing seems potentially inflexible to me. Here is a possible alternative:

  • tasks.yml provides the path to a task definition file (which could be YAML)
  • The task definition file contains the platform, parameters, and additional information needed to process the task. The idea here would be to make the task definition file more modular; so simple tasks could just be a templated .travis.yml, but we wouldn't be constraining ourselves to that model

My guess is that as time goes on, more of the logic for each task will move from templates to Python code, as a means of improving code reuse.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've already lost in the YAML forest :)
I'm trying to update this PR do deploy the artifacts - hopefully until the sync, but these ci-test-loops are insanely time consuming.

@kszucs kszucs changed the title ARROW-2674: [Packaging] Start building nightlies ARROW-2676: [Packaging] Deploy build artifacts to github releases Jun 12, 2018
@kszucs
Copy link
Member Author

kszucs commented Jun 13, 2018

@wesm Rigth now I'm struggling with passing appveyor auth_tokens, but a couple of artifacts are already uploaded as release assets: https://github.com/kszucs/crossbow/releases

Here https://github.com/kszucs/crossbow/blob/build-30/job.yml is an example yml containing the submitted job's state.

There is draft for querying tasks statuses too.

@kszucs
Copy link
Member Author

kszucs commented Jun 13, 2018

@wesm @cpcloud Do we want to support other git hosting services or just github? Because we might consider to drop pygit2 dependency and use only github's API.

@cpcloud
Copy link
Contributor

cpcloud commented Jun 14, 2018

@kszucs Let's stick with github for now and we can extend to other if the need arises.

Copy link
Contributor

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

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

Looking mostly good! Couple of questions/comments.

@@ -343,6 +343,9 @@ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${ARROW_CXXFLAGS}")
# For any C code, use the same flags.
set(CMAKE_C_FLAGS "${CMAKE_CXX_FLAGS}")

# Remove --std=c++11 to avoid errors from C compilers
string(REPLACE "-std=c++11" "" CMAKE_C_FLAGS ${CMAKE_C_FLAGS})
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, this is a temporary fix until we can address ARROW-2720. This is fragile if (with CMAKE_CXX_STANDARD set to 11) -std=gnu++11 ends up in CMAKE_C_FLAGS.

Copy link
Member

Choose a reason for hiding this comment

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

This is removed in master (I removed it in a patch recently). I think this is a rebase artifact and should be removed

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, that removal broke osx builds though. That's why we added it back.

Copy link
Contributor

Choose a reason for hiding this comment

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

It broke osx wheel and/or conda builds, not CI builds (obviously) though.

Copy link
Member

Choose a reason for hiding this comment

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

That's super weird, how did that flag end up in CMAKE_C_FLAGS? What kind of error did it cause?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't exactly what's happening, but I know that we are setting CMAKE_CXX_STANDARD to 11 and then settings CMAKE_C_FLAGS to CMAKE_CXX_FLAGS, which might explain how the std flag is ending up there. However, this doesn't show up on any NIX systems except OSX which is why I'm not 100% sure what's happening.

Copy link
Contributor

Choose a reason for hiding this comment

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

The error you get is error: invalid argument '-std=c++11' not allowed with 'C/ObjC' when the ae.c file is being compiled during the plasma compilation. Without a mac to figure out what's going on, it's pretty hard for me to tell what the heck is happening.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see. I think gcc and clang just ignore the flag when compiling C code. This suggests we should be setting up the CMAKE_C_FLAGS separate from CXX. I'm opening a JIRA

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Great! Thanks.

@@ -106,7 +106,7 @@ The script does the following:
$ git clone https://github.com/kszucs/crossbow

$ cd arrow/dev/tasks
$ python crossbow.py
$ python crossbow.py submit <task names>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you give an example here instead of a placeholder?

@@ -115,7 +115,7 @@ The script does the following:

```bash
git checkout ARROW-<ticket number>
python dev/tasks/crossbow.py --dry-run
python dev/tasks/crossbow.py submit --dry-run <task names>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

@@ -124,85 +124,56 @@ The script does the following:
3. Reads and renders the required build configurations with the parameters
substituted.
2. Create a commit per build configuration to its own branch. For example
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still correct? Isn't it now "A branch per task, prefixed with the job id"?


# Configure conda.
- |
echo ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason why we're echoing a newline here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy paste from conda forge. This newline probably makes the output more readable.

wheel-linux \
wheel-win \
wheel-osx \
linux-packages
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a fine place to keep the nightly jobs list for now. There's nothing preventing us from moving this to a non-travis cron in the future if there's some reason we would need to do that.

# format="[%(asctime)s] %(levelname)s Crossbow %(message)s",
# datefmt="%H:%M:%S",
# stream=click.get_text_stream('stdout')
# )
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to just delete this code instead of leaving it here commented out?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, the previous solution used logging instead of click.echo. It was Wes' request, but generated a lot of noise, so I've switched to directly printing to stdout.
Honestly I'd remove it :)



class Target(object):
class Repo(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we really trying to be python 2 compatible?

def next_job_id(self, prefix):
"""Auto increments the branch's identifier based on the prefix"""
pattern = re.compile(prefix + '-(\d+)')
matches = list(filter(None, map(pattern.match, self.repo.branches)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there away to get the most recently created branch, from the server, so that we don't have to look at every branch that exists every time we create a new one?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not aware of any, but I don't think we should worry about that - with libgit2 traversing is really fast.
If it's going to be a problem, we can address it later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good.

provider: releases
api_key: $CROSSBOW_GITHUB_TOKEN
file_glob: true
file: /path/to/pachages/*.tar.gz
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

After the build passes.

@@ -106,7 +106,7 @@ The script does the following:
$ git clone https://github.com/kszucs/crossbow

$ cd arrow/dev/tasks
$ python crossbow.py
$ python crossbow.py submit <task names>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way that we can make the submit subcommand show a better error message if the task isn't valid?

Right now I get this:

$ python crossbow.py submit --dry-run linux-packges
Traceback (most recent call last):
  File "crossbow.py", line 409, in <module>
    crossbow(auto_envvar_prefix='CROSSBOW')
  File "/home/phillip/miniconda/envs/pyarrow36/lib/python3.6/site-packages/click/core.py", line 722, in __call__
    return self.main(*args, **kwargs)
  File "/home/phillip/miniconda/envs/pyarrow36/lib/python3.6/site-packages/click/core.py", line 697, in main
    rv = self.invoke(ctx)
  File "/home/phillip/miniconda/envs/pyarrow36/lib/python3.6/site-packages/click/core.py", line 1066, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/phillip/miniconda/envs/pyarrow36/lib/python3.6/site-packages/click/core.py", line 895, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/phillip/miniconda/envs/pyarrow36/lib/python3.6/site-packages/click/core.py", line 535, in invoke
    return callback(*args, **kwargs)
  File "crossbow.py", line 338, in submit
    tasks = {name: tasks[name] for name in task_names}
  File "crossbow.py", line 338, in <dictcomp>
    tasks = {name: tasks[name] for name in task_names}
KeyError: 'linux-packges'

Copy link
Contributor

Choose a reason for hiding this comment

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

Addressed in kszucs#3

return Job(tasks=tasks, branch=data.get('branch'))

def files(self):
return {'job.yml': yaml.dump(self.to_dict(), default_flow_style=False)}


# this should be the mailing list
MESSAGE_EMAIL = '[email protected]'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we instead 1) make this configurable via an environment variable, and 2) default to the email of the git user triggering the builds?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can submit a patch to your fork for these changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -80,12 +80,12 @@ submission. The tasks are defined in `tasks.yml`
6. Install the python dependencies for the script:

Copy link
Contributor

Choose a reason for hiding this comment

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

One thing that is worth nothing in this README.md is that you need at least one commit in your queue repo or else this happens:

Traceback (most recent call last):
  File "crossbow.py", line 413, in <module>
    crossbow(auto_envvar_prefix='CROSSBOW')
  File "/home/phillip/miniconda/envs/pyarrow36/lib/python3.6/site-packages/click/core.py", line 722, in __call__
    return self.main(*args, **kwargs)
  File "/home/phillip/miniconda/envs/pyarrow36/lib/python3.6/site-packages/click/core.py", line 697, in main
    rv = self.invoke(ctx)
  File "/home/phillip/miniconda/envs/pyarrow36/lib/python3.6/site-packages/click/core.py", line 1066, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/phillip/miniconda/envs/pyarrow36/lib/python3.6/site-packages/click/core.py", line 895, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/phillip/miniconda/envs/pyarrow36/lib/python3.6/site-packages/click/core.py", line 535, in invoke
    return callback(*args, **kwargs)
  File "crossbow.py", line 356, in submit
    queue.put(job)
  File "crossbow.py", line 186, in put
    branch = self._create_branch(task.branch, files=task.files())
  File "crossbow.py", line 158, in _create_branch
    parents = parents or [self.head]
  File "crossbow.py", line 96, in head
    return self.repo.head.target
_pygit2.GitError: reference 'refs/heads/master' not found

Is there any way that we can automate this away? If not, make sure to add a note about it in the README.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need parent commits, it just makes the graph nicer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Resolved

@cpcloud cpcloud self-requested a review June 18, 2018 20:00
@kszucs
Copy link
Member Author

kszucs commented Jun 18, 2018

@cpcloud https://github.com/kszucs/crossbow/releases/tag/build-95 26 artifacts, is that correct?

(not counting linux packages)

@cpcloud
Copy link
Contributor

cpcloud commented Jun 18, 2018

@kszucs There should be two each of pyarrow and arrow-cpp windows packages: one for python 3.5 and one for python 3.6 for a total of 4 windows conda packages + parquet-cpp

@cpcloud
Copy link
Contributor

cpcloud commented Jun 19, 2018

Restarted the travis build

@kszucs
Copy link
Member Author

kszucs commented Jun 20, 2018

@cpcloud All of the conda and whl artifacts are uploaded now https://github.com/kszucs/crossbow/releases
Linux pkgs afe still failing, here is the tracking issue https://issues.apache.org/jira/browse/ARROW-2713

@cpcloud
Copy link
Contributor

cpcloud commented Jun 20, 2018

@wesm @xhochy @kszucs has artifacts for all platforms and pythons here if you're interested: https://github.com/kszucs/crossbow/releases/tag/build-115

Copy link
Contributor

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

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

This looks good to me. @kszucs Thanks for all of your work on this, I know it's been an uphill battle with continuous integration services.

This is really going to streamline the release process going forward.

@wesm @xhochy If you have any more comments, I'd like to merge this today or tomorrow.

@cpcloud
Copy link
Contributor

cpcloud commented Jun 20, 2018

@kszucs Do we have a JIRA for this?

We can determine whether all the expected artifacts from a particular run have been successfully uploaded (i.e. to GitHub)

- for %%f in (*.tar.bz2) do ren "%%f" "%%~nf-win-64.tar.bz2"
- for %%f in (*.tar.bz2) do (
set %%g=%%~nf
ren "%%f" "%%~ng-win-64.tar.bz2"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. Batch is truly horrifying sometimes.

Copy link
Member Author

Choose a reason for hiding this comment

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

grgrgr

@kszucs
Copy link
Member Author

kszucs commented Jun 20, 2018

@cpcloud Created JIRA for that https://issues.apache.org/jira/browse/ARROW-2724

@cpcloud
Copy link
Contributor

cpcloud commented Jun 22, 2018

@wesm I'm ready to merge if you are.

There are a couple of follow up tasks:

  1. Setup nightlies on travis (which will trigger runs on both appveyor and travis). https://issues.apache.org/jira/browse/ARROW-1581 and https://issues.apache.org/jira/browse/ARROW-1582.
  2. https://issues.apache.org/jira/browse/ARROW-2724: Determine if all artifacts are successfully uploaded.

Copy link
Member

@wesm wesm left a comment

Choose a reason for hiding this comment

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

+1 to merge and move on to the next stage of work. We need update dev/release/RELEASE_MANAGEMENT.md -- maybe when this is done you should advise the mailing list so some others can take this for a test run and see what issues they run into

@cpcloud
Copy link
Contributor

cpcloud commented Jun 24, 2018

@pcmoritz @robertnishihara Have either of you encountered this error before: https://travis-ci.org/apache/arrow/jobs/395810684

I'm guessing that we happened to get a machine with very little memory on that particular run. Wondering if this is transient or an honest-to-goodness bug.

@cpcloud
Copy link
Contributor

cpcloud commented Jun 24, 2018

Arg, restarting the build appears to overwrite the previous build's log.

@cpcloud
Copy link
Contributor

cpcloud commented Jun 24, 2018

Ok, merging.

@cpcloud cpcloud closed this in 58a2366 Jun 24, 2018
@cpcloud
Copy link
Contributor

cpcloud commented Jun 24, 2018

Thanks @kszucs! Onwards!

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

Successfully merging this pull request may close these issues.

4 participants