Skip to content

Conversation

@rjernst
Copy link
Member

@rjernst rjernst commented Oct 26, 2024

The libs projects are configured to all begin with elasticsearch-. While this is desireable for the artifacts to contain this consistent prefix, it means the project names don't match up with their directories. Additionally, it creates complexities for subproject naming that must be manually adjusted.

This commit adjusts the project names for those under libs to be their directory names. The resulting artifacts for these libs are kept the same, all beginning with elasticsearch-.

The libs projects are configured to all begin with `elasticsearch-`.
While this is desireable for the artifacts to contain this consistent
prefix, it means the project names don't match up with their
directories. Additionally, it creates complexities for subproject naming
that must be manually adjusted.

This commit adjusts the project names for those under libs to be their
directory names. The resulting artifacts for these libs are kept the
same, all beginning with `elasticsearch-`.
@rjernst rjernst added :Delivery/Build Build or test infrastructure >refactoring labels Oct 26, 2024
@rjernst rjernst requested review from a team as code owners October 26, 2024 14:01
@elasticsearchmachine elasticsearchmachine added the Team:Delivery Meta label for Delivery team label Oct 26, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

}
c.getOutputs().doNotCacheIf("BWC distribution caching is disabled for local builds", task -> BuildParams.isCi() == false);
c.getArgs().add(projectPath.replace('/', ':') + ":" + assembleTaskName);
c.getArgs().add("-p");
Copy link
Member Author

Choose a reason for hiding this comment

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

By using the directory name (which hasn't changed) this allows for bwc builds to run, even though this change has not yet been backported.


testImplementation(project(":test:framework")) {
exclude group: 'org.elasticsearch', module: 'elasticsearch-core'
exclude group: 'org.elasticsearch', module: 'core'
Copy link
Member Author

Choose a reason for hiding this comment

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

Does the gradle "module name" matter? This PR produces the same artifact ids, though the "name" attribute in the pom is different. Is there a way to control this module name in gradle? I could only find archivesName which we set as the artifactId.

Copy link
Contributor

Choose a reason for hiding this comment

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

The "name" is purely descriptive so I think it's fine if they are different. So long as the artifactId is the same we won't have to deal with updating dependencies.

name: Projects tend to have conversational names, beyond the artifactId. The Sun engineers did not refer to their project as "java-1.5", but rather just called it "Tiger". Here is where to set that value.

@rjernst rjernst added v8.17.0 auto-backport Automatically create backport pull requests when merged labels Oct 26, 2024
':server:generatePluginsList',
':generateProviderImpls',
':libs:elasticsearch-native:elasticsearch-native-libraries:extractLibs',
':libs:native:libraries:extractLibs',
Copy link
Contributor

Choose a reason for hiding this comment

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

Why has elasticsearch-native-libraries becomes simply libraries?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just the local project name. Before we manually changed it to elasticsearch-native-libraries in settings.gradle. Now the artifact remains the same (set recursively in libs/build.gradle), but the local name is simplified.

}

if (project.name == 'log4j') {
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just wrap the last bit in a conditional. These conditionals that rely on returns are very easy to get removed, or other logic being added below that should apply, but doesn't.

apply plugin: 'base'

def baseProject = project
def baseName = "elasticsearch-${it.name}"
Copy link
Contributor

Choose a reason for hiding this comment

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

The term "base name" doesn't really make sense here anymore. Can we call this artifactId instead? I think that describes what where' trying to influence here.

archivesName = baseName
}
subprojects {
plugins.withType(JavaPlugin).whenPluginAdded {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not gonna lie, I struggle to follow what we're trying to do here. Can we at least add some clarifying comments here to convey not just what we're doing, but why this is necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added comments for the entire section explaining what we are doing.

@elasticsearchmachine elasticsearchmachine added the serverless-linked Added by automation, don't add manually label Oct 28, 2024
@rjernst
Copy link
Member Author

rjernst commented Oct 29, 2024

@mark-vieira I believe this is all working now and I've addressed your comments.

Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

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

LGTM

@rjernst rjernst merged commit e5d5c17 into elastic:main Oct 29, 2024
17 checks passed
@rjernst rjernst deleted the build/libs-names branch October 29, 2024 20:02
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.x Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 115720

rjernst added a commit to rjernst/elasticsearch that referenced this pull request Oct 30, 2024
The libs projects are configured to all begin with `elasticsearch-`.
While this is desireable for the artifacts to contain this consistent
prefix, it means the project names don't match up with their
directories. Additionally, it creates complexities for subproject naming
that must be manually adjusted.

This commit adjusts the project names for those under libs to be their
directory names. The resulting artifacts for these libs are kept the
same, all beginning with `elasticsearch-`.
elasticsearchmachine pushed a commit that referenced this pull request Oct 30, 2024
* Use directory name as project name for libs (#115720)

The libs projects are configured to all begin with `elasticsearch-`.
While this is desireable for the artifacts to contain this consistent
prefix, it means the project names don't match up with their
directories. Additionally, it creates complexities for subproject naming
that must be manually adjusted.

This commit adjusts the project names for those under libs to be their
directory names. The resulting artifacts for these libs are kept the
same, all beginning with `elasticsearch-`.

* fixes
jfreden pushed a commit to jfreden/elasticsearch that referenced this pull request Nov 4, 2024
The libs projects are configured to all begin with `elasticsearch-`.
While this is desireable for the artifacts to contain this consistent
prefix, it means the project names don't match up with their
directories. Additionally, it creates complexities for subproject naming
that must be manually adjusted.

This commit adjusts the project names for those under libs to be their
directory names. The resulting artifacts for these libs are kept the
same, all beginning with `elasticsearch-`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged backport pending :Delivery/Build Build or test infrastructure >refactoring serverless-linked Added by automation, don't add manually Team:Delivery Meta label for Delivery team v8.17.0 v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants