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

Import target determinator and test suite #1

Merged
merged 10 commits into from
May 9, 2022

Conversation

illicitonion
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

Are there going to be git submodules??

.github/workflows/presubmit.yml Outdated Show resolved Hide resolved
/bazel-*
/.ijwb/

/third_party/protobuf/bazel/*/*.pb.go
Copy link
Collaborator

Choose a reason for hiding this comment

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

if these files are needed to make the editor happy, you could check them in

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They're not required, it's more of a "If you wanted to see the output" - IntelliJ works fine for me without the manual generation.

WORKSPACE Outdated Show resolved Hide resolved
WORKSPACE Outdated Show resolved Hide resolved
WORKSPACE Outdated Show resolved Hide resolved
scripts/generate-proto Outdated Show resolved Hide resolved
scripts/update-dependencies Show resolved Hide resolved
Author: Daniel Wagner-Hall <dwagnerhall@apple.com>
Date: Thu Apr 21 18:21:46 2022 +0100

Only return rules (and not external ones)
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there an upstream PR for it yet? Ideally that PR number would go in the filename of this patch, so it's easier for us to track when it's merged and the patch can be dropped.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is not - I'll happily kick off a discussion async :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

sounds good, I often just create the draft PR at least as a placeholder so I don't forget to start the process of converging again with upstream

import java.util.Set;
import org.junit.Ignore;

public class BazelDiffIntegrationTest extends Tests {
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you add a README explaining whether this is testing against all three implementations?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added in 08765c6 :)

@illicitonion
Copy link
Collaborator Author

Are there going to be git submodules??

I wasn't intending for there to be - should there be?

Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

Is there really not a "comment on file" feature in GitHub? I asked about git submodules because there was an empty .gitmodules file at the root but you've removed it :)

Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

I've barely scanned the .go and .java files based on my understanding that they were already reviewed in another repo where the code originated.

README.md Outdated Show resolved Hide resolved
scripts/format Show resolved Hide resolved
@@ -0,0 +1,138 @@
# This file was manually copied from https://github.com/ewhauser/bazel-differ/blob/916e6409ae501c56ac4d6da9c0785c2ee97721b3/deps.bzl because gazelle doesn't seem to like working with dep files from remote repositories.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've had this issue recently as well, was it related to select clauses in generated BUILD files?
aspect-build/bazel-examples#4 (comment)

One of us should file upstream on gazelle...

@alexeagle
Copy link
Collaborator

oh and we should figure out why the tests aren't triggering on this PR

name: presubmit
on:
push:
pull_request:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this empty block is the problem with no checks running

@illicitonion
Copy link
Collaborator Author

oh and we should figure out why the tests aren't triggering on this PR

I think GitHub Actions can't be added in a PR from another fork, or something like that. They've been running on my fork: https://github.com/illicitonion/target-determinator/runs/6323004650?check_suite_focus=true and I expect them to start running on merge :)

@illicitonion illicitonion merged commit 21d163e into bazel-contrib:main May 9, 2022
@illicitonion illicitonion deleted the initial-export branch May 9, 2022 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants