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

2.12 update #428

Closed
wants to merge 12 commits into from
Closed

2.12 update #428

wants to merge 12 commits into from

Conversation

ittaiz
Copy link
Member

@ittaiz ittaiz commented Feb 20, 2018

this is just master with cherry-picking 08f3ca5 thanks to @hmemcpy who grouped together the 2.12 relevant changes to ease this branch update

ittaiz and others added 12 commits January 18, 2018 09:47
* jmh normalizes jar timestamps
build_is_identical e2e test spits out differences in md5

* change jar creator to default to normalizing timestamps
)

* Output statsfile with line 0 = compilation time

* Actually specify that the statsfile is an output :)

* Use Java nio utilities to write statsfile; adjust to property file inspired syntax

* Add a test for the statsfile output
…lbuild#409)

When executing non-locally, the wrong version of Java would be used,
referencing a path that didn't exist. This change adds the
_java_runtime attribute to have to locate the java runtime on the
machine where the Scala code is being executed, and selects the
correct configuration for the host_javabase.

As a minor cleanup, this also removes the now no-longer-necessary
_java attribute.

TESTED=Ran unit tests and also ran a custom Scala unit test through:
previously, the Scala unit test failed with an incorrect java path
error, now it passes.

Change-Id: I9c2829bded04ec2d9d014094bd9e72cd984fe6e6
The IntelliJ plugin assumes that source_jar and source_jars are both
present on the jars struct. This sets those to empty values, so the
IntelliJ plugin does not fail.
Without it, builds fail on Windows as it uses semi-colon instead.
* use transitive_proto_path_flags if exist

* Update scala_proto.bzl
@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@ittaiz
Copy link
Member Author

ittaiz commented Feb 20, 2018

note re cla- I verified it's all approved authors since it's just master with manually taken changes by @hmemcpy

@johnynek
Copy link
Member

👍

@ittaiz
Copy link
Member Author

ittaiz commented Feb 21, 2018

I want to keep the history close to master and not squash the commits but this means force pushing into 2.12.
@johnynek wdyt?

@johnynek
Copy link
Member

I’m confused about this diff since it seems to have diffs that are already in 2.12. For instance I don’t know why the hash change on the jars since that was already updated.

Anyway, I view 2.12 branch as an alpha type branch.

That said, if you force push, I worry if that could break users using Shas now on the branch. I don’t actually know what happens to the shas. I guess they will still be accessible.

@ittaiz
Copy link
Member Author

ittaiz commented Feb 21, 2018

I'm indeed not sure why they are showing up in the diff.
I'm also not sure what will happen to the sha but from a small experimentation I did on a different repo it seems they are still accessible (although I thought they wouldn't be).
I think we can maintain this in two ways:

  1. Merge master into this branch constantly- the pro is that this is the git way to do this but the con is that it creates unclear history (to me) and we already saw regressions sneak in.
  2. Cherry-pick the 2.12 commit on top of master each time and force push here- less clean and unclear on why we're seeing that diff.
  3. Cherry-pick the 2.12 commit on top of master each time and push to a new branch each time master_$date_2.12- not the git way but solves the above issues.

WDYT?
Internally we use option 3 for rules_scala HEAD because we also cherry-pick a few other things we have yet been able to iron out

@johnynek
Copy link
Member

johnynek commented Feb 21, 2018 via email

@ittaiz
Copy link
Member Author

ittaiz commented Feb 22, 2018

ok,
@anchlovi will start working on having it selectable on master next week using the plan we discussed in #399 and to lesser extent in #407
So I'm keeping this open but not merging it for the time being

@ittaiz
Copy link
Member Author

ittaiz commented Mar 9, 2018

Seems like superseded by #435

@ittaiz ittaiz closed this Mar 9, 2018
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.

7 participants