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.

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

Rework the ReadMe to be more quick-start friendly #1173

Merged
merged 35 commits into from
Dec 26, 2024

Conversation

stefankiesz
Copy link
Contributor

@stefankiesz stefankiesz commented May 6, 2024

What was changed?
The ReadMe was re-worked to be more quick-start friendly. Missing information was added and made into tables. Typos and grammar issues were addressed.
(More details in the Overview section below.)

Why was it changed?
To improve the ReadMe and correct outstanding issues.

How was it changed?
By modifying the ReadMe document to address the above issues.

What testing was done for the changes?
The document was pasted into a spelling and grammar checker.


Overview

  • Add a collapsible vertical table of contents
  • Add a Quick Start section
  • Extend samples instructions
  • Add missing Windows instructions
  • Fix incorrect dependency installation commands
  • Add more links to resources and references
    • New links include: AWS KVS getting started page, AWS KVS FAQs page, GStreamer element docs, Java Producer SDK
    • Also added a link to Producer C in the Key Features section incase user does not need JNI nor GStreamer features
  • Make the CMake Arguments section into a table and fix typos in the descriptions
  • Make the kvssink parameters into a table, fix missing links, add in aws-region parameter
  • Mention and show how to stream from file with the kvssink sample
  • To the Quick Start section, add instructions for how/where to view the footage from the samples and link troubleshoot guides
  • ...more misc. changes

Future Improvements

  • Consider splitting the QuickStart instructions for building the SDK into build-with-dependencies a build-using-system-dependencies and demonstrate how to install the system dependencies and the correct versions.
  • Improve upon and add to the Debugging/Troubleshooting section - likely should add more links to AWS docs. Specify Java version for JNI. Also should probably add a link to Java SDK and instructions if any are missing on that repo's ReadMe.
  • Add installation commands to all the prerequisite packages such as git and cmake. This will likely only fit well if the QuickStart section gets restructured into 3 separate sections for each OS.
  • Add a "Running Tests" section to show how to build and run the tests. Include details such as AWS authentication and how resources are managed when tests are running and failing.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

README.md Show resolved Hide resolved

If you are building on Windows you need to generate `NMake Makefiles`, you should run `cmake .. -G "NMake Makefiles"`
_Mac and Linux_
Copy link
Contributor

Choose a reason for hiding this comment

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

Would suggest splitting this into 2 subsections:

  1. Build from scratch (build with dependencies=ON)
  2. Build using system libraries (build with dependencies=OFF). For this, you would also need to install the necessary dependencies using system package manager

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going to add this to the PR description as a future addition to consider, but I'm not set on doing this quite yet since for the smoothest onboarding experience we would recommend to do build dependencies to ensure a supported version of a dependency library is being used.

Copy link
Contributor Author

@stefankiesz stefankiesz Jul 3, 2024

Choose a reason for hiding this comment

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

To add to that, it's not as simple as just including the install instructions with the correct versions: users might already have some of the dependency libraries installed with unsupported versions and there would be potential for CMake or pkgconfig to link the incorrect ones.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 105 lines in your changes missing coverage. Please review.

Project coverage is 16.30%. Comparing base (eeb392f) to head (03d8f84).
Report is 10 commits behind head on develop.

Current head 03d8f84 differs from pull request most recent head 3bb045b

Please upload reports for the commit 3bb045b to get more accurate results.

Files Patch % Lines
samples/kvssink_gstreamer_sample.cpp 0.00% 50 Missing ⚠️
...mazonaws/kinesis/video/producer/jni/Parameters.cpp 0.00% 46 Missing ⚠️
src/gstreamer/gstkvssink.cpp 0.00% 8 Missing ⚠️
...s/video/producer/jni/KinesisVideoClientWrapper.cpp 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1173      +/-   ##
===========================================
- Coverage    16.48%   16.30%   -0.18%     
===========================================
  Files           50       50              
  Lines         7019     7095      +76     
===========================================
  Hits          1157     1157              
- Misses        5862     5938      +76     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

README.md Outdated Show resolved Hide resolved
@stefankiesz stefankiesz marked this pull request as ready for review July 3, 2024 19:21
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@stefankiesz stefankiesz requested a review from sirknightj July 9, 2024 05:14
README.md Outdated
_Windows_
```bat
choco install gstreamer --version=1.22.8
choco install gstreamer-devel --version=1.22.8
Copy link
Contributor

Choose a reason for hiding this comment

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

How come the gstreamer version is only specified in the windows? Does mac and linux requires certain gstreamer versions? Is this the only supported gstreamer version for windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Likely not the only supported version, but for the windows build, we have relied on using the pkg-config library bundled with the gstreamer installation. We've found that some versions of gstreamer do not have the pkg-config library. I believe we may be able to switch to just using pkg-config-lite, installed via choco, then we can remove this version specifier.

README.md Outdated Show resolved Hide resolved
@stefankiesz stefankiesz changed the base branch from develop-pre-3.4.1 to develop December 26, 2024 19:14
@stefankiesz stefankiesz changed the base branch from develop to develop-pre-3.4.1 December 26, 2024 19:16
@stefankiesz stefankiesz changed the base branch from develop-pre-3.4.1 to readme-rework-pre-3.4.1 December 26, 2024 19:19
@stefankiesz stefankiesz merged commit 12830f1 into readme-rework-pre-3.4.1 Dec 26, 2024
stefankiesz added a commit that referenced this pull request Dec 26, 2024
* Start remodeling the ReadMe

* Fix typo, add italics to OS names

* add todo

* Finish up the body

* Update ToC

* Move stuff, fix typos

* Improve title

* Add a line

* Add link for cmake options

* Fix ToC

* Pi->Linux

* Add a comma

* Address feedback

* Add more Windows instructions

* Restructure Debugging section, add more links

* Add some periods

* Add link to Java SDK

* Add link to Producer C SDK in Key Features section

* Convert cmake options list to table

* Add kvssink parameters table, modify credential-path description, cleanup cmake table

* Add stream from file kvssink sample instructions

* Add instructions for how/where to view the footage from the samples in the Quick Start section

* Address comments

* Spelling and grammar

* Remove accidental link

* Downgrade Mac CI GCC version 13->12

* Add AWS Authentication to quickstart section

* megabytes -> mebibytes

* Address comments

* Fix paths

* Format element names

* Remove code formatting from links

* Format fixing

* Update README.md

* Update README.md
stefankiesz added a commit that referenced this pull request Jan 10, 2025
* Rework the ReadMe to be more quick-start friendly (#1173)

* Start remodeling the ReadMe

* Fix typo, add italics to OS names

* add todo

* Finish up the body

* Update ToC

* Move stuff, fix typos

* Improve title

* Add a line

* Add link for cmake options

* Fix ToC

* Pi->Linux

* Add a comma

* Address feedback

* Add more Windows instructions

* Restructure Debugging section, add more links

* Add some periods

* Add link to Java SDK

* Add link to Producer C SDK in Key Features section

* Convert cmake options list to table

* Add kvssink parameters table, modify credential-path description, cleanup cmake table

* Add stream from file kvssink sample instructions

* Add instructions for how/where to view the footage from the samples in the Quick Start section

* Address comments

* Spelling and grammar

* Remove accidental link

* Downgrade Mac CI GCC version 13->12

* Add AWS Authentication to quickstart section

* megabytes -> mebibytes

* Address comments

* Fix paths

* Format element names

* Remove code formatting from links

* Format fixing

* Update README.md

* Update README.md

* Address comments

* Address comments

* Add link to AWS KVS Raspberry Pi doc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants