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

Some improvements to tox-bootstrapd's Dockerfile #1211

Merged
merged 2 commits into from
Oct 17, 2018

Conversation

nurupo
Copy link
Member

@nurupo nurupo commented Sep 27, 2018

Users probably shouldn't run toxcore master, running the highest version tag sounds better.

Switched it to use cmake to build toxcore instead of sing autotools while I was at it, since we are supposedly phasing out autotools at some point (maybe?).


This change is Reviewable

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@nurupo nurupo force-pushed the cmake-tag-bootstrapd branch 2 times, most recently from b20eb7d to 37bd08c Compare September 27, 2018 05:32
@iphydf iphydf modified the milestones: v0.2.8, v0.2.x Oct 7, 2018
Copy link
Member

@iphydf iphydf left a comment

Choose a reason for hiding this comment

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

:lgtm_strong:

Reviewed 4 of 4 files at r1.
Reviewable status: 0 of 1 approvals obtained

@nurupo nurupo force-pushed the cmake-tag-bootstrapd branch 2 times, most recently from 37bd08c to cd2bab2 Compare October 11, 2018 22:47
@nurupo
Copy link
Member Author

nurupo commented Oct 11, 2018

Rebased and CI passes. Removing the CI enabling commit.

@nurupo nurupo force-pushed the cmake-tag-bootstrapd branch from cd2bab2 to 8bef9fc Compare October 11, 2018 22:56
@iphydf iphydf changed the title [WIP] Some improvements to tox-bootstrapd's Dockerfile Some improvements to tox-bootstrapd's Dockerfile Oct 16, 2018
@iphydf
Copy link
Member

iphydf commented Oct 16, 2018

LGTM - feel free to rebase and merge (I can't do it for you).

@nurupo nurupo force-pushed the cmake-tag-bootstrapd branch from 8bef9fc to 58f2b92 Compare October 17, 2018 01:38
@codecov
Copy link

codecov bot commented Oct 17, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1211     +/-   ##
========================================
- Coverage      83%   82.9%   -0.1%     
========================================
  Files          82      82             
  Lines       14692   14645     -47     
========================================
- Hits        12195   12143     -52     
- Misses       2497    2502      +5
Impacted Files Coverage Δ
toxcore/TCP_server.c 75.9% <0%> (-4.4%) ⬇️
toxcore/Messenger.c 86.5% <0%> (-0.6%) ⬇️
toxcore/DHT.c 76.6% <0%> (-0.2%) ⬇️
toxcore/onion_client.c 95.9% <0%> (-0.1%) ⬇️
toxav/toxav.c 67.3% <0%> (ø) ⬆️
toxcore/net_crypto.c 94.8% <0%> (ø) ⬆️
toxav/msi.c 64.2% <0%> (+0.2%) ⬆️
toxcore/group.c 76.4% <0%> (+0.3%) ⬆️
auto_tests/save_load_test.c 98.6% <0%> (+0.5%) ⬆️
toxcore/friend_connection.c 94.5% <0%> (+1.4%) ⬆️
... and 2 more

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 f45bf68...b6dde3c. Read the comment docs.

@nurupo
Copy link
Member Author

nurupo commented Oct 17, 2018

Done.

Thanks for removing the WIP.

Have you noticed that while Dockerfile checks out the greatest version number tag, the CI script seds the Dockerfile to run tests against the current changes instead? You should have seen that if you checked the patch, though I decided to point it out. I was looking for some Dockerfile condition thingy where it would do different thing if CI=true env variable is set, but alas there is no such functionality, at least not in the building stage of the container.

@iphydf
Copy link
Member

iphydf commented Oct 17, 2018

Yes, I noticed that when I was adding a test for the alpine dockerfile (which I then deleted - and I dropped the PR for testing the dockerfile). LGTM => rebase.

@nurupo nurupo force-pushed the cmake-tag-bootstrapd branch from 58f2b92 to b6dde3c Compare October 17, 2018 18:46
@nurupo
Copy link
Member Author

nurupo commented Oct 17, 2018

Rebased.

@iphydf iphydf merged commit b6dde3c into TokTok:master Oct 17, 2018
@robinlinden robinlinden modified the milestones: v0.2.x, v0.2.9 Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants