Skip to content

Conversation

pledbrook
Copy link
Contributor

DO NOT MERGE!

This is a series of changes to the Working With Files chapter of the user guide that I'm collecting into a pull request to make reviewing easier. Hopefully the changes will eventually propagate to the user guide, but for now they should not be merged.

I have started the chapter with a series of common tasks that users want to
accomplish with files. The aim is to answer some commonly asked questions
while directing readers to more detailed information so that they have a
broader understanding of the topic.
Copy link
Contributor

@bmuschko bmuschko left a comment

Choose a reason for hiding this comment

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

Hi Peter,

I ran over the preliminary content and left a couple of comments. Overall, the content is already much better than before. I am going to give a more thorough pass when you think the changes are in a somewhat final state.

== Working With Files

Most builds work with files. Gradle adds some concepts and APIs to help you achieve this.
Almost every Gradle build works with files: think source files, library dependencies, reports and so on. That's why Gradle comes with a comprehensive API that makes it simple to achieve common tasks.
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of "library dependencies" we settled on the term "file dependency". See https://docs.gradle.org/nightly/userguide/declaring_dependencies.html#sec:declaring_file_dependency for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Use "file dependency" in place of "library dependency"

== Working With Files

Most builds work with files. Gradle adds some concepts and APIs to help you achieve this.
Almost every Gradle build works with files: think source files, library dependencies, reports and so on. That's why Gradle comes with a comprehensive API that makes it simple to achieve common tasks.
Copy link
Contributor

Choose a reason for hiding this comment

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

In this section we can probably be more concrete. I'd say "file operations" instead of "common tasks".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Be more concrete and less ambiguous than "common tasks"


You copy a file by creating an instance of Gradle's builtin api:org.gradle.api.tasks.Copy[] task and configuring it with the location of the file and where you want to put it. This example mimics copying a generated report into a directory that will be packed into an archive (zip, tarball, or whatever):

----
Copy link
Contributor

Choose a reason for hiding this comment

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

All code samples should be an embedded code snippet from an actual build script under docs/src/samples. Furthermore, all of them should be tested e.g. underneath integ-test/src/integTest/groovy/org/gradle/integtests/samples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood. I simply wanted to get the text done first. BTW, are there any instructions for running the docs integration tests or a subset of them? I remember working it out last year, but have of course forgotten and can't find any info on it.

Copy link
Contributor Author

@pledbrook pledbrook Feb 16, 2018

Choose a reason for hiding this comment

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

  • Turn code examples into tested samples
  • Update existing samples to use best practice and reflect their corresponding points as succinctly and well as possible

Copy link
Contributor

@bmuschko bmuschko Feb 16, 2018

Choose a reason for hiding this comment

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

There are two types of tests you can run:

  1. The UserGuideSamplesIntegrationTest which basically only verifies that the expected output has been produced according to the *.out that follow the naming convention of the sample ID. Instructions are in the comment of the test class.
  2. The corresponding integration test(s) that execute the sample code and make assertions about the outcome. One example is SamplesDeclaringDependenciesIntegrationTest. After extracting the samples (as described in 1) you can just run it from the IDE. It's possible that they do not exist for this page. I'd set up a new test class if it doesn't exist yet.

When reworking content in the dependency management chapter I have been leaning mostly toward 2 as it provides a way verify more than just the output.

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'm running into an issue trying to execute the dedicated integration tests from IntelliJ:

java.lang.ExceptionInInitializerError
	at org.gradle.integtests.fixtures.AbstractIntegrationSpec.$spock_initializeFields(AbstractIntegrationSpec.groovy:56)
Caused by: org.gradle.internal.service.ServiceCreationException: Could not create service of type ClassLoaderRegistry using GlobalScopeServices.createClassLoaderRegistry().
	at org.gradle.integtests.fixtures.executer.AbstractGradleExecuter.<clinit>(AbstractGradleExecuter.java:92)
	... 1 more
Caused by: java.lang.IllegalArgumentException: Cannot find JAR 'slf4j-api-1.7.16.jar' required by module 'gradle-core' using classpath.
	at org.gradle.api.internal.classpath.DefaultModuleRegistry.findDependencyJar(DefaultModuleRegistry.java:254)
	... 9 more

I imported the the project after running ./gradlew idea. Not sure what else I need to do. Any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hhhm, doesn't sounds familiar. Can you try the following?

  1. Run ./gradlew cleanIdea idea from the command line. Do not import the project by pointing to the root build.gradle file. There're still some things that don't work properly yet with the built-in Gradle import functionality. Just open the generated gradle.ipr file.
  2. Run ./gradlew intTestImage from the command line.
  3. Run the test from the IDE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I ran into another issue with a conflicting groovy-all library, but hacked that to get the tests running from within the IDE.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, great. Saw your question on the channel. Looks like it was a bug.


We look at these parts in more detail later in the chapter, but we'll start with how you accomplish some of the most common tasks users need.

=== Copying a single file
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 would make sense to also explain the from/into methods to a certain extent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, although I'll keep it minimal. Now that I have the later sections in place, it will be easier to reference them appropriately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Explain what from() and into() do/are for

Note that hardcoding paths like this makes the build brittle. It's better to use the task that generates a file as the source of the file's location. In this modified example, we use a report task defined elsewhere that has the report's location stored in the `outputFile` property:

----
ext.archiveDir = file("${buildDir}/toArchive")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd define the extra property for the task and not the project. That will provide a narrower context and can only be used with the reference of this specific task. Ultimately, it avoids introducing global properties which makes the build harder to maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I was thinking in terms of a generic directory where all archives are put, but that's not the case here. It's specific to this task.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Move project property onto the task


----
task packageDistribution(type: Zip) {
archiveName = "my-distribution.zip"
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should promote the use of the base plugin here instead as it provides the conventions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since there are two examples, I'd like to have one without the plugin, to demonstrate that it's not needed, and one with to show the convenience. I'll probably trim this section a bit too, as I've now updated the in-depth section on creating archives which covers the base plugin and naming conventions in detail.

Copy link
Contributor Author

@pledbrook pledbrook Feb 16, 2018

Choose a reason for hiding this comment

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

  • Use one sample without the Base Plugin, and one with

}
----

=== Moving files and directories
Copy link
Contributor

Choose a reason for hiding this comment

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

Good to spell it out!

}
----

As you can see, you can have multiple `from()` declarations in a copy specification, each with its own configuration.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have a section that explain the file operation methods on Project (e.g. project.copy) and the difference between the task types and the methods.

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 was already in the chapter, but I've updated it. You'll find it in the File Copying in Depth section. It's actually focused on the copy() method, but now you mention it, there isn't anything on delete() or its corresponding task right now. Are there any others I should cover?

Copy link
Contributor Author

@pledbrook pledbrook Feb 16, 2018

Choose a reason for hiding this comment

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

Add information on the following:

  • Delete task and method
  • sync() method

Copy link
Contributor

Choose a reason for hiding this comment

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

I can only think of Project.mkdir() and Project.delete() right now. I don't think we need to show examples for all of them. It's probably OK to just list them and link to the DSL guide.


One thing to note is how the `include()` directive applies only to the `from()`, whereas the directive in the previous section applied to the whole task. These different levels of granularity in the copy specification allow you to easily handle most requirements that you will come across. TODO: Link to more details on copy specs.

=== Creating archives (zip, tar, etc.)
Copy link
Contributor

Choose a reason for hiding this comment

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

Frequently asked about on the user forum: How do I create an uberjar? It would be helpful to explain it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. If it's that frequently asked, it might make sense to include it as one of the early examples in the chapter, with perhaps some more detail in the in-depth section.

Copy link
Contributor Author

@pledbrook pledbrook Feb 16, 2018

Choose a reason for hiding this comment

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

  • Provide an example on creating an uber JAR

@bmuschko Do you mean one in which the contents of the other JARs are included in the fat JAR, or the JARs themselves are included as is in the fat JAR (the Spring Boot approach — or at least it was).

Copy link
Contributor

@bmuschko bmuschko Feb 16, 2018

Choose a reason for hiding this comment

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

I meant 1. For example:

task uberjar(type: Jar) {
    from zipTree(jar1)
    from zipTree(jar2)
}

2 is similar except that the contents of the JAR files (to be included) are not extracted via zipTree.


This is not a common requirement and should be used sparingly as you lose information and can easily break a build. It's generally preferable to copy directories and files instead.

=== Renaming files on copy
Copy link
Contributor

Choose a reason for hiding this comment

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

One example on more complex processing would be great as well e.g. the use of eachFile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a common scenario for users in which something like eachFile() is the best solution? Or is it only for specialised cases?

@pledbrook
Copy link
Contributor Author

@bmuschko @eriwen Are we using the guides style guide on the user manual? Is there another document? I hate not knowing how to refer to plugins by name, whether to capitalize archive format names, and so on.

@bmuschko
Copy link
Contributor

Are we using the guides style guide on the user manual? Is there another document? I hate not knowing how to refer to plugins by name, whether to capitalize archive format names, and so on.

The guides style guide contains some good advice. I don't think we have been following it in the past when writing user guide content. It would make sense to enhance the style guide by additional rules and then use it across the board. @eriwen Any additional thoughts?


attributes website: 'http://www.gradle.org'
attributes website: 'http://www.gradle.org',
javaApi: 'https://docs.oracle.com/javase/8/docs/api'
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to reference the Java 7 docs here as we still supporting it as the minimal JDK version.

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'm fine with that. One of the reasons I made it a document attribute :)

Copy link
Contributor Author

@pledbrook pledbrook Feb 21, 2018

Choose a reason for hiding this comment

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

  • Reference Java 7 API docs rather than Java 8

Copy link
Contributor

@eriwen eriwen left a comment

Choose a reason for hiding this comment

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

Overall, these are incredible updates. I want to ship this as soon as possible, even if it's not completely done, because I think the ~40000 users who will visit this page between Gradle 4.6 and 4.7 will really appreciate it. There are only a couple fixes necessary for this, which I've marked with an ❌


----
task copyReports(type: Copy) {
from "${buildDir}/reports/my-report.pdf", "src/docs/manual.pdf"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use myReportTask.outputFile here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Explicit file paths are easier to understand, so I prefer them in simple examples. But I do think we need to warn against the use of hard-coded paths in more places than is currently done.

  • Warn against the use of hard-coded paths in all appropriate places within the chapter

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good

}
----

This approach has the side effect of copying the directory structure below `reports` as well as the files. This may be what you want. It may not be. There are many such decisions you need to make when copying files, but rest assured that there are very few requirements that can't be handled elegantly by Gradle copy specifications.
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be what you want. It may not be. There are many such decisions you need to make when copying files

I weakly feel that this particular text doesn't really help users much. I think we can be more succinct here.

Copy link
Contributor Author

@pledbrook pledbrook Feb 23, 2018

Choose a reason for hiding this comment

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

Yes, I'm not particularly happy with it. Will revisit.

  • Reword above

You may have the need to copy not just files, but the directory structure they reside in as well. This is, in fact, the default behavior when you specify a directory as the `from()` argument. So the following will copy everything in the `reports` directory, including all its subdirectories:

----
task copyReports(type: Copy) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As we convert these to samples, I expect these bits will have meaningful example subtitles, which will be useful for those folks just skimming this doc.


----
task copyReports(type: Copy) {
from "${buildDir}/reports"
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing I've always found confusing is how much the order of these things matter. I think it'd be nice to be explicit whether include must come before into or that it doesn't matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another option is simply to assume order independence and call out when order does matter. That reflects my own view that this is configuration and should inherently be order independent. Making this explicit everywhere in the manual might be a little over the top. Not sure.

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 okay with that approach as I don't feel strongly here

One thing to note is how the `include()` directive applies only to the `from()`, whereas the directive in the previous section applied to the whole task. These different levels of granularity in the copy specification allow you to easily handle most requirements that you will come across. You can learn more about this in the section on <<Using child specifications,child specifications>>.

[[sec:creating_archives_example]]
=== Creating archives (zip, tar, etc.)
Copy link
Contributor

Choose a reason for hiding this comment

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

Very good title


As you can see, the api:org.gradle.api.file.CopySpec#with(org.gradle.api.file.CopySpec...)[] method is the key to incorporating an existing copy specification into another one.

===== Using child specifications
Copy link
Contributor

Choose a reason for hiding this comment

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

Very good.

One final thing to be aware of is that a child copy spec inherits its destination path, include patterns, exclude patterns, copy actions, name mappings and filters from its parent. So be careful where you place your configuration.

[[sec:project_copy_method]]
==== Copying files in your own tasks
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this deserves it's own subsection because it feels like a common use case. That is, use of === Copying ... to ensure it gets it's own spot in the side navigation.

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 know. It would certainly be easier for readers to find currently if they could browse the second-level headings as well. Even if it doesn't make sense to do this in the side nav, I think having a TOC at the front of the chapter would help.

On a general note, I'm worried that we'll end up with a suboptimal structure because we're trying to satisfy a constraint of only top-level headings being visible in the navigation. It makes browsing and discovery harder than it would otherwise be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm...I thought I wrote something on this, but I can't remember where. I feel like the single-level heading in the navigation is forcing a structure that isn't that easy for readers to parse and comprehend. Using the copy method is still technically file copying and it would be confusing to lift it up a level just so that it appears in the side nav. And I would use the same argument for file collections and file trees, both of which should be discoverable without a page search in my view.

If I have talked about this elsewhere, sorry for repeating myself 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, browser had a cached copy of the page without my previous comments in it. Sorry about that!

TODO: Use a generic archive example. No need to use Java here.

[TIP]
.Why are you using the Java plugin?
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a reminder to remove this if you use an example that doesn't need the Java plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done