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

Fix typo in readme file #2132

Merged
merged 3 commits into from
Jan 18, 2022
Merged

Fix typo in readme file #2132

merged 3 commits into from
Jan 18, 2022

Conversation

joostlek
Copy link
Member

@joostlek joostlek commented Jan 9, 2022

Summary

Screenshots

Link to pull request in Documentation repository

Documentation: home-assistant/companion.home-assistant#

Any other notes

@homeassistant
Copy link
Contributor

Hi @joostlek,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@joostlek
Copy link
Member Author

After following the readme I found a few things that went different. Should I include them into this PR and make this a readme improvement PR or should I create a new PR for that

@dshokouhi
Copy link
Member

After following the readme I found a few things that went different. Should I include them into this PR and make this a readme improvement PR or should I create a new PR for that

Feel free to include them in here 🙂

@joostlek
Copy link
Member Author

After following the readme I found a few things that went different. Should I include them into this PR and make this a readme improvement PR or should I create a new PR for that

Feel free to include them in here 🙂

Updated! When I generated my keystore, I got an error which said it ignored my alias password because it did not support it. Can you test if you have this too? If that happens it will default to the keystore password. I can imagine having a hard time figuring out why a build would fail.

@dshokouhi
Copy link
Member

Updated! When I generated my keystore, I got an error which said it ignored my alias password because it did not support it. Can you test if you have this too? If that happens it will default to the keystore password. I can imagine having a hard time figuring out why a build would fail.

hmm I do not have a keystore file in those location and I do not have any issues with builds running.

@joostlek
Copy link
Member Author

Updated! When I generated my keystore, I got an error which said it ignored my alias password because it did not support it. Can you test if you have this too? If that happens it will default to the keystore password. I can imagine having a hard time figuring out why a build would fail.

hmm I do not have a keystore file in those location and I do not have any issues with builds running.

What OS are you using?

@dshokouhi
Copy link
Member

i am on windows 10

@joostlek
Copy link
Member Author

joostlek commented Jan 10, 2022

i am on windows 10

Do you have the KEYSTORE_PATH env set to something? Apparently it defaults to the release_keystore.keystore.

        create("release") {
            storeFile = file(System.getenv("KEYSTORE_PATH") ?: "release_keystore.keystore")
            storePassword = System.getenv("KEYSTORE_PASSWORD") ?: ""
            keyAlias = System.getenv("KEYSTORE_ALIAS") ?: ""
            keyPassword = System.getenv("KEYSTORE_ALIAS_PASSWORD") ?: ""
            enableV1Signing = true
            enableV2Signing = true
        }
    }

@dshokouhi
Copy link
Member

i am on windows 10

Do you have the KEYSTORE_PATH env set to something? Apparently it defaults to the release_keystore.keystore.

        create("release") {
            storeFile = file(System.getenv("KEYSTORE_PATH") ?: "release_keystore.keystore")
            storePassword = System.getenv("KEYSTORE_PASSWORD") ?: ""
            keyAlias = System.getenv("KEYSTORE_ALIAS") ?: ""
            keyPassword = System.getenv("KEYSTORE_ALIAS_PASSWORD") ?: ""
            enableV1Signing = true
            enableV2Signing = true
        }
    }

ok I only get this issue when I build for release personally I never have a need for that....When you look at the left side window you will see "Build Variants" click on that and change it to one of the debug releases and it will work. I never used release since I dont use them for development.

image

@joostlek
Copy link
Member Author

Does gradlew build default to the release variant? And if we only build the debug builds, why bother making a keystore?

@dshokouhi
Copy link
Member

Does gradlew build default to the release variant?

looks like it does

And if we only build the debug builds, why bother making a keystore?

I think its for the forks?

@joostlek
Copy link
Member Author

Does gradlew build default to the release variant?

looks like it does

And if we only build the debug builds, why bother making a keystore?

I think its for the forks?

Doesn't that mean we can remove that step from the setup. (Or place it under a "If you want to fork and sign it"). And change the gradle command to one that just builds the debug ones.

@joostlek
Copy link
Member Author

gradlew assembleDebug only builds the debug applications, so it doesn't require a Keystore. So it would be that or having people specify the path to their keystore (which they probably won't use)

@dshokouhi
Copy link
Member

Good points :) lets wait for another reviewer so we can make an appropriate decision here

@JBassett
Copy link
Collaborator

Does gradlew build default to the release variant?

It actually builds and tests all variants

gradlew assembleDebug only builds the debug applications, so it doesn't require a Keystore. So it would be that or having people specify the path to their keystore (which they probably won't use)

Breaking down the steps further probably makes sense. Most people contributing only need debug builds. I would break this into two sections I guess. Development, what most people are going to do. Then a preparing for publishing, which we can mention that is only for those who publish the application.

@joostlek
Copy link
Member Author

When generating a signed APK, should we just say follow the intellij tutorial or also include the gradlew build part with the environment variables?

Copy link
Member

@dshokouhi dshokouhi left a comment

Choose a reason for hiding this comment

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

Thank you for cleaning this up!

@dshokouhi dshokouhi merged commit 9a28be2 into home-assistant:master Jan 18, 2022
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.

4 participants