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

bazel-4.1.0rc4 ignores exec_compatible_with for some of cc_test actions #13450

Closed
glukasiknuro opened this issue May 9, 2021 · 8 comments
Closed
Labels
P0 This is an emergency and more important than other current work. (Assignee required) team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website type: bug

Comments

@glukasiknuro
Copy link
Contributor

glukasiknuro commented May 9, 2021

Description of the problem / feature request:

When using cc_test in 4.1.0rc4 looks like build and linking actions are using platform honoring exec_compatible_with, but test and post-process actions (test-setup.sh and generate-xml.sh) are using platform that does not have constraint required by exec_compatible_with. Versions 3.7.2 and 4.0.0 seem to work correctly, 4.1.0rc4 and current master do not.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Created project demonstrating the issue in https://github.com/glukasiknuro/bazel_platforms_issue

git clone [email protected]:glukasiknuro/bazel_platforms_issue.git

cd bazel_platforms_issue/

sed -i 's/Fake test.*"/Fake test '$(date +%s)'"/g' platforms_test/main.cc && \
  ~/apps/bazel/bazel-4.1.0rc4-linux-x86_64 test \
    --extra_execution_platforms=//platforms_test:test_platform_no_constraint,//platforms_test:test_platform_with_constraint \
    //platforms_test:test_requiring_test_constraint_value  \
    --execution_log_json_file=/tmp/exec_platforms_test.json && \
  grep --after=4  '"platform"' /tmp/exec_platforms_test.json

Gives:

 "platform": {
    "properties": [{
      "name": "exec_property",
      "value": "requires_test_constraint"
    }]
--
  "platform": {
    "properties": [{
      "name": "exec_property",
      "value": "requires_test_constraint"
    }]
--
  "platform": {
    "properties": [{
      "name": "exec_property",
      "value": "no_constraint"
    }]
--
  "platform": {
    "properties": [{
      "name": "exec_property",
      "value": "no_constraint"
    }]

Note the first two actions have requires_test_constraint in platform properties, but the last two do not have it, but exec_compatible_with is specified on entire cc_test: - https://github.com/glukasiknuro/bazel_platforms_issue/blob/f0cee6458bee6ee5a668503f301dbfa3b8e27aa8/platforms_test/BUILD#L32

For bazel 4.0.0 or 3.7.2 all four actions have requires_test_constraint.

Replace this line with your answer.

What operating system are you running Bazel on?

Ubuntu 16.04

What's the output of bazel info release?

$ ~/apps/bazel/bazel-4.1.0rc4-linux-x86_64 info release
release 4.1.0rc4

Any other information, logs, or outputs that you want to share?

Found issue when trying to migrate bigger codebase, when some actions were executed on incompatible remote execution workers, while it worked previously.
Possibly a blocker for 4.1 release? #13099

@glukasiknuro
Copy link
Contributor Author

Bisect identified 3486b78 as changing behavior (CC @katre @philwo )

@sventiffe sventiffe added team-Rules-CPP Issues for C++ rules untriaged labels May 10, 2021
@oquenchil oquenchil added P3 We're not considering working on this, but happy to review a PR. (No assignee) type: bug and removed untriaged labels May 10, 2021
@philwo
Copy link
Member

philwo commented May 10, 2021

@oquenchil Why is this marked as P3 when it's a potential release blocker for Bazel 4.1.0? 🤔

@philwo philwo added P0 This is an emergency and more important than other current work. (Assignee required) release blocker team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website and removed P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-CPP Issues for C++ rules labels May 10, 2021
@oquenchil
Copy link
Contributor

@katre @gregestren Do you know any relatively recent changes that might have caused this?

@katre
Copy link
Member

katre commented May 10, 2021

There is a known issue with exec group inheritance. I need to scrub the internal issue and export it publicly.

Good news: I have a fix in progress.
Bad news: it's too complex to cherrypick to an RC, so we're better off removing the cherrypick of 1e258d2 from the release.

@katre
Copy link
Member

katre commented May 10, 2021

Note: I haven't had a chance to confirm that the issue is the same, but it's very likely based on the symptoms.

@philwo
Copy link
Member

philwo commented May 10, 2021

I checked with @katre, we will have to revert the commits in 4.1.0rc4:

FYI @quval @scele

@katre
Copy link
Member

katre commented May 10, 2021

I have confirmed that this is the same as #13459, and that the in progress fix for that also fixes your repro.

Thanks for the clear directions.

I hope to have #13459 finished in the next day or so.

@philwo philwo mentioned this issue May 10, 2021
9 tasks
@katre
Copy link
Member

katre commented May 12, 2021

#13459 is now closed since I submitted 645c42b.

We can't easily get this into 4.1, we'll have to decide how much this is needed for 4.2.

@katre katre closed this as completed May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 This is an emergency and more important than other current work. (Assignee required) team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website type: bug
Projects
None yet
Development

No branches or pull requests

5 participants