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

Update commit id in Dockerfile to trigger update. #2467

Merged
merged 1 commit into from
Dec 8, 2016

Conversation

TeBoring
Copy link
Contributor

@TeBoring TeBoring commented Dec 8, 2016

No description provided.

@TeBoring
Copy link
Contributor Author

TeBoring commented Dec 8, 2016

@stanley-cheung

@@ -132,7 +132,7 @@ ENV MVN mvn --batch-mode
RUN cd /tmp && \
git clone https://github.com/google/protobuf.git && \
cd protobuf && \
git reset bf379715c93b581eeb078cec1f0dd8a7d79df431 && \
git reset 46ae90dc5e145b12fffa7e053a908a9f3e066286 && \
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to change this commit id if Java dependencies are not changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#2435 changed php's dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

This line is building the cache for Java. If you haven't changed any Java dependencies, you don't need to change this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line is actually shared by both java and php. php also needs to download github repository.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you instead add a "git reset ..." on the line that builds php dependencies? That would be much clearer than changing it here.

@TeBoring
Copy link
Contributor Author

TeBoring commented Dec 8, 2016

The failed test is for ruby, which is not related to this PR.

@TeBoring TeBoring force-pushed the master branch 4 times, most recently from ab6bd26 to 0692e50 Compare December 8, 2016 21:59
@@ -473,7 +473,9 @@ def test_map_basic
m2 = m.dup
assert_equal m, m2
assert m.hash != 0
assert_equal m.hash, m2.hash
# TODO(teboring): Disable jruby test temperarily for it randomly fails.
Copy link
Contributor

Choose a reason for hiding this comment

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

This test case is ran under both regular ruby and jruby. You are disabling both here. Could you just ignore the jruby test?

@TeBoring TeBoring merged commit e3e38b8 into protocolbuffers:master Dec 8, 2016
TeBoring added a commit that referenced this pull request Dec 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants