-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add clang-format 7.0.1 to CI image #1719
Add clang-format 7.0.1 to CI image #1719
Conversation
Also, is |
Yes, this image is also used as a base for the reinforcement_learning repo where cpprestsdk is used. Traditionally we've targeted older versions of linux to ensure compatibility. I think there will be less resistance to simply installing a newer clang-format on the 14.04 image if that is possible. |
What is involved in getting the required version of clang-format into the current ubuntu image? |
Probably one of
I'll try one of them tonight |
Status on this? It would be nice to avoid needing the latest and greatest to build VW. |
Sorry, haven't made much time for this yet. I do think it is possible and
will try this weekend at the latest. Is this blocking anything other than
the formatting work?
…On Thu, Jan 24, 2019, 5:32 AM John ***@***.*** wrote:
Status on this? It would be nice to avoid needing the latest and greatest
to build VW.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1719 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEL88XKEz4yiRLzrI-N9MZMLmojNqViaks5vGbYDgaJpZM4aJaBH>
.
|
It's not blocking, but it would be nice to land a big patch like this
because it keeps getting obsolete as the underlying codebase changes.
…-John
On Thu, Jan 24, 2019 at 1:11 PM Palmer Lao ***@***.***> wrote:
Sorry, haven't made much time for this yet. I do think it is possible and
will try this weekend at the latest. Is this blocking anything other than
the formatting work?
On Thu, Jan 24, 2019, 5:32 AM John ***@***.*** wrote:
> Status on this? It would be nice to avoid needing the latest and greatest
> to build VW.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <
#1719 (comment)
>,
> or mute the thread
> <
https://github.com/notifications/unsubscribe-auth/AEL88XKEz4yiRLzrI-N9MZMLmojNqViaks5vGbYDgaJpZM4aJaBH
>
> .
>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1719 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAE25przLAoQjs8mc0c9pTU7eNRicpdMks5vGfdMgaJpZM4aJaBH>
.
|
cb2e518
to
b4457dc
Compare
b4457dc
to
b3200aa
Compare
@@ -1,8 +1,7 @@ | |||
FROM ubuntu:14.04 AS build | |||
|
|||
# Upgrade cmake to 3.2 | |||
RUN apt-get update | |||
RUN apt-get install -y software-properties-common python-software-properties debconf-utils | |||
RUN apt-get update && apt-get install -y software-properties-common python-software-properties debconf-utils |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand very well why this change was necessary, but without it, running docker build - < .pipelines/Dockerfile
gave me an error on this step that looked like apt-get update
hadn't been run. Combining the two lines made it work again.
The Docker documentation indicates that this seems to be the preferable way to run apt-get update
.
Friendly reminder @JohnLangford Is this something you all are still interested in? The change is a fair bit smaller now and I think uses the same versions of other software as before. |
Yep merged, thanks :-) (I was just locked on the COLT deadline until yesterday.) |
* Make Dockerfile build again * Download clang-format
This installs
clang-format
7.0.1 (it seems to me 7.x.x is considered "stable" LLVM).