Skip to content

Commit

Permalink
Improve overlapping test source directories
Browse files Browse the repository at this point in the history
  • Loading branch information
@guw
guw committed Jul 29, 2023
1 parent ea87570 commit 4b4c100
Show file tree
Hide file tree
Showing 7 changed files with 129 additions and 40 deletions.
16 changes: 15 additions & 1 deletion ...alesforce.bazel.eclipse.core/src/com/salesforce/bazel/eclipse/core/model/BazelTarget.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ public BazelRuleAttributes getRuleAttributes() throws CoreException {
* in case of errors loading the rules info
*/
public String getRuleClass() throws CoreException {
return getInfo().getTarget().getRule().getRuleClass();
return getInfo().getRule().getRuleClass();
}

/**
Expand Down Expand Up @@ -164,4 +164,18 @@ public boolean hasBazelProject() throws CoreException {
public int hashCode() {
return Objects.hash(bazelPackage, targetName);
}

/**
* Returns <code>true</code> if the target is a rule and has a attribute named <code>tags</code> containing the
* specified tag.
*
* @param tag
* the tag to check
* @return <code>true</code> if the rule has the specified tag, <code>false</code> otherwise
* @throws CoreException
*/
public boolean hasTag(String tag) throws CoreException {
var tags = getInfo().getRuleAttributes().getStringList("tags");
return (tags != null) && tags.contains(tag);
}
}
3 changes: 2 additions & 1 deletion ...l.eclipse.core/src/com/salesforce/bazel/eclipse/core/model/SynchronizeProjectViewJob.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,8 @@ private static String queryForTargets(Collection<TargetExpression> targets) {
return targetList;
}

return String.format("attr('tags', '^((?!manual).)*$', %s)", targetList);
// do not exclude the manual tags here but make sure to filter out "no-ide" targets
return String.format("attr('tags', '^((?!no-ide).)*$', %s)", targetList);
}

private final BazelWorkspace workspace;
Expand Down
37 changes: 29 additions & 8 deletions ....core/src/com/salesforce/bazel/eclipse/core/model/discovery/BaseProvisioningStrategy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ private void addResourceFolders(BazelProject project, List<IClasspathEntry> rawC
}

private void addSourceFolders(BazelProject project, List<IClasspathEntry> rawClasspath,
JavaSourceInfo javaSourceInfo, boolean useTestsClasspath) {
JavaSourceInfo javaSourceInfo, boolean useTestsClasspath) throws CoreException {
var virtualSourceFolder = useTestsClasspath ? getFileSystemMapper().getVirtualSourceFolderForTests(project)
: getFileSystemMapper().getVirtualSourceFolder(project);
var outputLocation = useTestsClasspath ? getFileSystemMapper().getOutputFolderForTests(project).getFullPath()
Expand All @@ -207,13 +207,34 @@ private void addSourceFolders(BazelProject project, List<IClasspathEntry> rawCla
var sourceFolder = dir.isEmpty() ? virtualSourceFolder : project.getProject().getFolder(dir);
var inclusionPatterns = javaSourceInfo.getInclusionPatternsForSourceDirectory(dir);
var exclusionPatterns = javaSourceInfo.getExclutionPatternsForSourceDirectory(dir);
rawClasspath.add(
JavaCore.newSourceEntry(
sourceFolder.getFullPath(),
inclusionPatterns,
exclusionPatterns,
outputLocation,
classpathAttributes));
var existingEntry =
rawClasspath.stream().anyMatch(entry -> entry.getPath().equals(sourceFolder.getFullPath()));
if (existingEntry) {
if (useTestsClasspath) {
createBuildPathProblem(
project,
Status.warning(
format(
"Folder '%s' found twice on the classpath. This is likely because it's used as test as well as non-test resource. Please consider modifying the project setup!",
sourceFolder)));
} else {
createBuildPathProblem(
project,
Status.error(
format(
"Folder '%s' found twice on the classpath. This is an unexpected situation. Please consider modifying the project setup! Don't hesitate and reach out for help.",
sourceFolder)));
}
} else {
rawClasspath.add(
JavaCore.newSourceEntry(
sourceFolder.getFullPath(),
inclusionPatterns,
exclusionPatterns,
outputLocation,
classpathAttributes));
}

}
}
}
Expand Down
13 changes: 12 additions & 1 deletion ...core/src/com/salesforce/bazel/eclipse/core/model/discovery/BazelQueryTargetDiscovery.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
*/
public class BazelQueryTargetDiscovery implements TargetDiscoveryStrategy {

private static final String TAG_NO_IDE = "no-ide";

public static final String STRATEGY_NAME = "bazel-query";

private static Logger LOG = LoggerFactory.getLogger(BazelQueryTargetDiscovery.class);
Expand Down Expand Up @@ -102,7 +104,7 @@ public Collection<BazelTarget> discoverTargets(BazelWorkspace bazelWorkspace,
bazelPackage.getLabel(),
bazelPackage.getBazelTargets());
}
targets.addAll(bazelPackage.getBazelTargets());
bazelPackage.getBazelTargets().stream().filter(this::isVisibleToIde).forEach(targets::add);
monitor.worked(1);
}

Expand Down Expand Up @@ -147,4 +149,13 @@ protected IPath findWorkspaceLocation(BazelWorkspace bazelWorkspace, IPath packa
}
return findWorkspaceLocation(bazelWorkspace, packagePath.removeLastSegments(1));
}

private boolean isVisibleToIde(BazelTarget bazelTarget) {
try {
return !bazelTarget.hasTag(TAG_NO_IDE);
} catch (CoreException e) {
LOG.error("Error reading tags for target '{}': {}", bazelTarget, e.getMessage(), e);
return true;
}
}
}
52 changes: 29 additions & 23 deletions .../salesforce/bazel/eclipse/core/model/discovery/ProjectPerPackageProvisioningStrategy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import com.salesforce.bazel.eclipse.core.model.BazelWorkspace;
import com.salesforce.bazel.eclipse.core.model.discovery.classpath.ClasspathEntry;
import com.salesforce.bazel.eclipse.core.model.discovery.projects.JavaSourceEntry;
import com.salesforce.bazel.eclipse.core.model.discovery.projects.JavaSourceInfo;
import com.salesforce.bazel.sdk.aspects.intellij.IntellijAspects.OutputGroup;
import com.salesforce.bazel.sdk.command.BazelBuildWithIntelliJAspectsCommand;
import com.salesforce.bazel.sdk.model.BazelLabel;
Expand Down Expand Up @@ -178,6 +179,32 @@ public Map<BazelProject, Collection<ClasspathEntry>> computeClasspaths(Collectio
}
}

private void createWarningsForUnsupportedLayouts(BazelPackage bazelPackage, List<BazelTarget> packageTargets,
BazelProject project, JavaSourceInfo sourceInfo) throws CoreException {
if (sourceInfo.hasSourceFilesWithoutCommonRoot()) {
for (JavaSourceEntry file : sourceInfo.getSourceFilesWithoutCommonRoot()) {
createBuildPathProblem(
project,
Status.warning(
format(
"File '%s' could not be mapped into a common source directory. The project may not build successful in Eclipse.",
file.getPath())));
}
}
if (!sourceInfo.hasSourceDirectories()) {
createBuildPathProblem(
project,
Status.info(
format(
"No source directories detected when analyzing package '%s' using targets '%s'",
bazelPackage.getLabel().getPackagePath(),
packageTargets.stream()
.map(BazelTarget::getLabel)
.map(BazelLabel::getLabelPath)
.collect(joining(", ")))));
}
}

@Override
protected List<BazelProject> doProvisionProjects(Collection<BazelTarget> targets, SubMonitor monitor)
throws CoreException {
Expand Down Expand Up @@ -208,29 +235,8 @@ protected List<BazelProject> doProvisionProjects(Collection<BazelTarget> targets
var javaInfo = collectJavaInfo(project, packageTargets, monitor.split(1));

// sanity check
var sourceInfo = javaInfo.getSourceInfo();
if (sourceInfo.hasSourceFilesWithoutCommonRoot()) {
for (JavaSourceEntry file : sourceInfo.getSourceFilesWithoutCommonRoot()) {
createBuildPathProblem(
project,
Status.warning(
format(
"File '%s' could not be mapped into a common source directory. The project may not build successful in Eclipse.",
file.getPath())));
}
}
if (!sourceInfo.hasSourceDirectories()) {
createBuildPathProblem(
project,
Status.info(
format(
"No source directories detected when analyzihng package '%s' using targets '%s'",
bazelPackage.getLabel().getPackagePath(),
packageTargets.stream()
.map(BazelTarget::getLabel)
.map(BazelLabel::getLabelPath)
.collect(joining(", ")))));
}
createWarningsForUnsupportedLayouts(bazelPackage, packageTargets, project, javaInfo.getSourceInfo());
createWarningsForUnsupportedLayouts(bazelPackage, packageTargets, project, javaInfo.getTestSourceInfo());

// configure classpath
configureRawClasspath(project, javaInfo, monitor.split(1));
Expand Down
2 changes: 1 addition & 1 deletion ....core/src/com/salesforce/bazel/eclipse/core/model/discovery/projects/JavaProjectInfo.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ public IStatus analyzeProjectRecommendations(IProgressMonitor monitor) throws Co
resourceInfo = new JavaResourceInfo(resources, bazelPackage);
resourceInfo.analyzeResourceDirectories(result);

testSourceInfo = new JavaSourceInfo(this.testSrcs, bazelPackage.getLocation());
testSourceInfo = new JavaSourceInfo(this.testSrcs, bazelPackage.getLocation(), sourceInfo);
testSourceInfo.analyzeSourceDirectories(result);

testResourceInfo = new JavaResourceInfo(testResources, bazelPackage);
Expand Down
46 changes: 41 additions & 5 deletions ...e.core/src/com/salesforce/bazel/eclipse/core/model/discovery/projects/JavaSourceInfo.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@
*/
public class JavaSourceInfo {

private static final IPath INVALID = new Path("_not_following_ide_standards_");
private static final IPath NOT_FOLLOWING_JAVA_PACKAGE_STRUCTURE =
new Path("_not_following_java_package_structure_");

private static boolean isJavaFile(java.nio.file.Path file) {
return isRegularFile(file) && file.getFileName().toString().endsWith(".java");
Expand All @@ -46,6 +47,7 @@ private static boolean isJavaFile(java.nio.file.Path file) {
private final Map<IPath, IPath> detectedPackagePathsByFileEntryPathParent = new HashMap<>();
private final Collection<Entry> srcs;
private final IPath bazelPackageLocation;
private final JavaSourceInfo sharedSourceInfo;

/**
* A list of all source files impossible to identify a common root directory
Expand All @@ -61,6 +63,26 @@ private static boolean isJavaFile(java.nio.file.Path file) {
public JavaSourceInfo(Collection<Entry> srcs, IPath bazelPackageLocation) {
this.srcs = srcs;
this.bazelPackageLocation = bazelPackageLocation;
this.sharedSourceInfo = null;
}

/**
* Use this constructor for test sources, i.e. sources which may have targets sharing sources.
* <p>
* Bazel allows to re-use sources in multiple targets. It will then compile those multiple times. An example setup
* is where all code is exposed as <code>java_library</code> as well as many targets for <code>java_test</code> with
* only one test class. If this is the case, we want to not issue "split package" warnings when the test class is
* already handled at the <code>java_library</code> level.
* </p>
*
* @param srcs
* @param bazelPackageLocation
* @param sharedSourceInfo
*/
public JavaSourceInfo(Collection<Entry> srcs, IPath bazelPackageLocation, JavaSourceInfo sharedSourceInfo) {
this.srcs = srcs;
this.bazelPackageLocation = bazelPackageLocation;
this.sharedSourceInfo = sharedSourceInfo;
}

@SuppressWarnings("unchecked")
Expand All @@ -84,7 +106,7 @@ public void analyzeSourceDirectories(MultiStatus result) throws CoreException {
"Java file '%s' (with detected package '%s') does not meet IDE standards. Please move into a folder hierarchy which follows Java package structure!",
fileEntry.getPath(),
fileEntry.getDetectedPackagePath())));
return INVALID;
return NOT_FOLLOWING_JAVA_PACKAGE_STRUCTURE;
}

// build second index of parent for all entries with a potential source root
Expand Down Expand Up @@ -169,7 +191,7 @@ public void analyzeSourceDirectories(MultiStatus result) throws CoreException {
// (eg., glob(["src/test/java/some/package/only/*.java"])
for (var potentialSourceRootAndSourceEntries : sourceEntriesBySourceRoot.entrySet()) {
var potentialSourceRoot = potentialSourceRootAndSourceEntries.getKey();
if (INVALID.equals(potentialSourceRoot)) {
if (NOT_FOLLOWING_JAVA_PACKAGE_STRUCTURE.equals(potentialSourceRoot)) {
continue;
}
if (!(potentialSourceRootAndSourceEntries.getValue() instanceof List)) {
Expand Down Expand Up @@ -198,11 +220,25 @@ public void analyzeSourceDirectories(MultiStatus result) throws CoreException {
}
}

/*
* Bazel allows to re-use sources in multiple targets. It will then compile those multiple times. An example setup
* is where all code is exposed as <code>java_library</code> as well as many targets for <code>java_test</code> with
* only one test class. If this is the case, we want to not issue "split package" warnings when the test class is
* already handled at the <code>java_library</code> level.
*/
if ((sharedSourceInfo != null) && sharedSourceInfo.hasSourceDirectories()) {
var sharedSourceDirectories = sharedSourceInfo.getSourceDirectories();
potentialSplitPackageOrSubsetFolders.removeIf(
potentialSourceRoot -> sharedSourceDirectories.contains(potentialSourceRoot)
|| sharedSourceDirectories.stream().anyMatch(p -> p.isPrefixOf(potentialSourceRoot)));
}

// when there are no split packages we found a good setup
if (potentialSplitPackageOrSubsetFolders.isEmpty()) {
// collect remaining files without a root
if (sourceEntriesBySourceRoot.containsKey(INVALID)) {
sourceFilesWithoutCommonRoot = (List<JavaSourceEntry>) sourceEntriesBySourceRoot.remove(INVALID);
if (sourceEntriesBySourceRoot.containsKey(NOT_FOLLOWING_JAVA_PACKAGE_STRUCTURE)) {
sourceFilesWithoutCommonRoot =
(List<JavaSourceEntry>) sourceEntriesBySourceRoot.remove(NOT_FOLLOWING_JAVA_PACKAGE_STRUCTURE);
}

// create source directories
Expand Down

0 comments on commit 4b4c100

Please sign in to comment.