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.

Sign up for GitHub

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

Jump to bottom

JLBP: Declare all dependencies #1767

Merged
merged 7 commits into from Mar 4, 2021
Merged

JLBP: Declare all dependencies #1767

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
246b15d
Declare all dependencies
elharo Oct 29, 2020
950379e
Merge branch 'master' into i1542
elharo Oct 29, 2020
af8178f
code review
elharo Oct 29, 2020
665e7d9
Merge branch 'master' into i1542
elharo Oct 30, 2020
6d9e34e
code review
elharo Oct 30, 2020
a6b9969
Merge branch 'master' into i1542
elharo Mar 4, 2021
4e19a5a
Merge branch 'master' into i1542
elharo Mar 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
58 changes: 58 additions & 0 deletions docs/JLBP-0022.md
View file
Open in desktop
@@ -0,0 +1,58 @@
---
jlbp:
id: JLBP-22
permalink: /JLBP-22
---

# Declare all direct dependencies

If your code references a class—for example, by invoking a method in that class—declare a dependency that includes that class in your pom.xml, build.gradle,
Copy link
Contributor

Choose a reason for hiding this comment

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

The varying line lengths make this hard to review. Can you run mdformat?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-bash: mdformat: command not found

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a formatting practice for referring to files? Should this be pom.xml or pom.xml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we've chosen one yet. Might be worth checking general google style and general cleanup.

or equivalent. In Bazel this practice is called "strict deps".

Code should not call methods, reference fields, or instantiate classes from _indirect_ dependencies. These are dependencies of the declared dependencies. Projects that rely on indirect dependencies tend to break in unexpected ways when direct dependencies are upgraded.
suztomo marked this conversation as resolved.
Show resolved Hide resolved

For example, your project might declare a dependency on the
Google HTTP Java Client which
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Google HTTP Java Client which
Google HTTP Java Client, which

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is a restrictive clause, but it's arguable.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it were restrictive, that would mean that there are several Google HTTP Java Clients, only one of which depends on Apache HTTP Components. Is that the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's one way of identifying restrictive subordinate clauses, but a little more generally, "A restrictive adjective clause, on the other hand, is essential to a sentence and should not be set off by commas" https://www.thoughtco.com/restrictive-and-nonrestrictive-adjective-clauses-1689689

I think this clause is indeed essential to the sentence.

A better reference than that will have to wait until I get back to the office to retrieve my Chicago Manual of Style. :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

The content of the clause is important to the meaning of the paragraph, but not essential to the sentence.

"For example, your project might declare a dependency on the Google HTTP Java Client."

That doesn't change the reader's understanding of what you mean by "the Google HTTP Java Client", although it leaves out an important detail about why you're talking about it.

Compare to your sentence above: "Projects that rely on indirect dependencies have an annoying habit of breaking in unexpected ways when direct dependencies are upgraded." If you got rid of the restrictive clause and changed it to "Projects have an annoying habit of breaking in unexpected ways when direct dependencies are upgraded", the reader wouldn't know which projects you were talking about.

Anyway, it's your document.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it needs a comma for the reason David explained:

If it were restrictive, that would mean that there are several Google HTTP Java Clients, only one of which depends on Apache HTTP Components

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point is "essential to a sentence," which clause this is. It is not whether it is essential to the meaning of the modified noun or noun phrase.

itself depends on the Apache HTTP Components. If so, it is possible to use the `org.apache.http.client.utils.URLEncodedUtils` class in your own project without
explicitly declaring a dependency on Apache HTTP Components. However, you should
add the dependency anyway so that if a future version of the
Google HTTP Java Client no longer depends on Apache HTTP Components, your code
still compiles. Strict dependencies also help static analysis tools better understand a project.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you add names of these static analysis tools you have in mind?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed

Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm asked from somebody about this static analysis tools, I cannot answer it. Would you help me in this regard?

Copy link
Contributor

@suztomo suztomo Oct 30, 2020 •
edited

Choose a reason for hiding this comment

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

"Strict deps" appears in a previous paragraph, but 'Strict dependencies" does not. Would you pick one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bazel does call it "strict deps" bit otherwise we try not to abbreviate

Copy link
Contributor

Choose a reason for hiding this comment

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

Bazel call it in that way, but this document does not name this practice.


Code breakages due to indirect dependencies don't just
happen when a library is upgraded. They can also
occur when dependency mediation selects a different version of a library's dependency
that does not include the necessary indirect dependency. Relying on indirect dependencies cause problems for clients with different dependency trees
that don't appear in your own code.

IDE autocomplete suggestions are a common way projects come to depend on
indirect dependencies. When importing a new class, most IDEs only look to see if it's present in the classpath, not whether it comes from a direct or indirect dependency.

The `mvn dependency:analyze` command lists dependencies a Maven project uses
but hasn't declared:

```
[WARNING] Used undeclared dependencies found:
[WARNING] org.apache.maven.resolver:maven-resolver-impl:jar:1.4.1:compile
[WARNING] org.apache.maven.resolver:maven-resolver-api:jar:1.6.1:compile
[WARNING] org.apache.maven:maven-core:jar:3.6.3:compile
[WARNING] org.apache.maven:maven-model-builder:jar:3.6.3:compile
```

These should be added to your pom.xml file.

The tool also lists dependencies the project declares but doesn't use:
suztomo marked this conversation as resolved.
Show resolved Hide resolved

```
[WARNING] Unused declared dependencies found:
[WARNING] com.google.cloud.tools:dependencies:jar:1.5.5-SNAPSHOT:compile
[WARNING] com.google.truth:truth:jar:1.0.1:test
[WARNING] junit:junit:jar:4.13.1:test
[WARNING] org.mockito:mockito-core:jar:3.5.15:test
```

However its analysis of which dependencies aren't used is not as accurate
as its analysis of which dependencies are used. In particular,
it reports dependencies used through reflection as unused, so be cautious when
removing any allegedly unused dependencies.