Skip to content

Improve Building from Source Instructions #2257

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

Closed

Conversation

helloworldless
Copy link

@helloworldless helloworldless commented Jan 24, 2021

Description

Improve Building from Source Instructions

Motivation and Context

Making a few things more explicit to help newcomers to the project. This is based on my own experience struggling with this a bit. I hope it will help others in the future.

Previously, the first command encountered under Building From Source was mvn clean install. It's highly likely that a newcomer will run the first command they see, and in my case, I ended up waiting 10 mins only for the build to fail. So this command is not a good option to lead with. For that reason, I created separate sections for building a single sub-project and building the whole project and made sure that the single sub-project section comes first.

Testing

N/A

Types of changes

Documentation (README) change only

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed
  • A short description of the change has been added to the CHANGELOG
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

Add a note to the Building From Source instructions to indicate
that Python 3 is a requirement for building the archetype-lambda
project. Add section about code generation and a tip to help
IntelliJ IDEA users on how to fix class not found errors for
generated code.
@codecov-io
Copy link

codecov-io commented Jan 24, 2021

Codecov Report

Merging #2257 (95fa1dd) into master (f48fa01) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2257   +/-   ##
=========================================
  Coverage     77.62%   77.63%           
  Complexity      367      367           
=========================================
  Files          1237     1237           
  Lines         38904    38904           
  Branches       3064     3064           
=========================================
+ Hits          30201    30204    +3     
+ Misses         7239     7236    -3     
  Partials       1464     1464           
Flag Coverage Δ Complexity Δ
unittests 77.63% <ø> (+<0.01%) 0.00 <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...ssdk/core/internal/async/FileAsyncRequestBody.java 84.90% <0.00%> (+0.94%) 0.00% <0.00%> (ø%)
...ty/internal/IdleConnectionCountingChannelPool.java 87.80% <0.00%> (+2.43%) 0.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f48fa01...95fa1dd. Read the comment docs.

@helloworldless
Copy link
Author

helloworldless commented Jan 25, 2021

By the way, it did occur to me that entire contents of Building From Source should be move to docs/GettingStarted.md and Building From Source should just have a link to docs/GettingStarted.md. I didn't even notice docs/GettingStarted.md until I was raising the PR. But I figure I should wait for feedback before getting too carried away 😃

@helloworldless
Copy link
Author

I made a few more updates based on testing the behavior by starting from scratch a few times.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@zoewangg
Copy link
Contributor

Hi @helloworldless, thank you for your time and effort to work on improving the docs! 🙂 We will take a look shortly.

@@ -116,16 +116,51 @@ See the [Set up the AWS SDK for Java][docs-setup] section of the developer guide

Once you check out the code from GitHub, you can build it using Maven.

Note: The `archetypes/archetype-lambda` project requires that you have Python 3 in your path as `python`.
Copy link
Member

Choose a reason for hiding this comment

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

Can you add this info in the archetype-lambda README instead of the main README?
https://github.com/aws/aws-sdk-java-v2/blob/master/archetypes/archetype-lambda/README.md

`target/generated-sources` of many of the projects in this repo.

In IntelliJ IDEA, the default setting of "Generated sources folders: Detect automatically"
under `File | Settings | Build, Execution, Deployment | Build Tools | Maven | Importing` should suffice.
Copy link
Member

Choose a reason for hiding this comment

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

I would avoid giving super specific instructions on menu paths because they can vary - for example, in my IntelliJ the Build, Execution, Deployment menu is not under File | Settings but under Preferences.

Maybe just mention Build, Execution, Deployment | Build Tools | Maven | Importing? Or maybe instruct to search Build Tools in the Search tool.

@dagnir
Copy link
Contributor

dagnir commented Apr 8, 2021

Going to close this for now since it hasn't been updated in some time. Please feel free to reopen it once the previous feedback has been addressed 😄 .

@dagnir dagnir closed this Apr 8, 2021
aws-sdk-java-automation added a commit that referenced this pull request Nov 16, 2022
…337a39a6c

Pull request: release <- staging/84cb5c82-1de3-4c88-8120-0a4337a39a6c
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.

5 participants