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

Prepare dies if there are no tags yet - fatal: No names found, cannot describe anything #342

Open
chadwhitacre opened this issue Jan 14, 2022 · 14 comments
Labels
bug Something isn't working

Comments

@chadwhitacre
Copy link
Member

Steps to Reproduce

  1. Start a new repo.
  2. Commit a simple .craft.yml file.
  3. Run craft prepare 0.0.0.

Expected Result

Successful completion.

Actual Result

$ craft prepare 0.0.0
ℹ Checking the local repository status...                                                                            11:45:29
ℹ Releasing version 0.0.0 from main                                                                                  11:45:29
ℹ Preparing to release the version: 0.0.0                                                                            11:45:29
ℹ Created a new release branch: "release/0.0.0"                                                                      11:45:29
ℹ Switched to branch "release/0.0.0"                                                                                 11:45:29
here

 ERROR  fatal: No names found, cannot describe anything.                                                             11:45:29


  
  at Object.action (/Users/chadwhitacre/workbench/getsentry/craft/dist/craft:154:11690)
  at Bae.exec (/Users/chadwhitacre/workbench/getsentry/craft/dist/craft:154:12163)
  at /Users/chadwhitacre/workbench/getsentry/craft/dist/craft:154:18969
  at new Promise (<anonymous>)
  at Xae.handleTaskData (/Users/chadwhitacre/workbench/getsentry/craft/dist/craft:154:18858)
  at Xae.<anonymous> (/Users/chadwhitacre/workbench/getsentry/craft/dist/craft:154:18435)
  at Generator.next (<anonymous>)
  at o (/Users/chadwhitacre/workbench/getsentry/craft/dist/craft:154:16995)
  at processTicksAndRejections (internal/process/task_queues.js:95:5)

$

Here's the call that fails:

craft/src/utils/git.ts

Lines 35 to 38 in 2ff325e

export async function getLatestTag(git: SimpleGit): Promise<string> {
// This part is courtesy of https://stackoverflow.com/a/7261049/90297
return (await git.raw('describe', '--tags', '--abbrev=0')).trim();
}

See:

$ git describe --tags --abbrev=0
fatal: No names found, cannot describe anything.
$
@chadwhitacre chadwhitacre added the bug Something isn't working label Jan 14, 2022
@kamilogorek
Copy link
Contributor

The getLatestTag is being used in two separate functions.

One is generateChangesetFromGit which is trivial to fix, as it internally calls git.log through getChangesSince. git.log that we currently use, accepts from and to range limits, however, when they are both skipped completely, it will query all commits 0..HEAD, which is what we want for the initial release.

Second, however, is runPreReleaseCommand, which is not doing much, it only calls the configured script. However, oldVersion is the 1st argument to the said script, followed by newVersion. When skipped, it will be filtered, and newVersion will be placed as 1st value, which can be really confusing and produce some broken releases.
We do also provide CRAFT_NEW_VERSION and CRAFT_OLD_VERSION env variables, but those are easy to detect for presence.

@chadwhitacre
Copy link
Member Author

In the second case when there is no old version we should build the changelog from the beginning of the repo.

@kamilogorek kamilogorek changed the title Prepare dies if there are no tags yet Prepare dies if there are no tags yet - fatal: No names found, cannot describe anything Aug 17, 2022
@kamilogorek
Copy link
Contributor

Quick workaround for posterity:

git checkout <reasonable-commit>
git tag 0.0.0
git push origin 0.0.0

@asottile-sentry
Copy link
Member

the publish diff makes a link against null... when using the placeholder 0.0.0 tag as well

@asottile-sentry
Copy link
Member

this is not so simple -- the prepare scripts pass the previous version string to the script hooks. so there must be something for those. the quick start guide currently recommends tagging the first commit as 0.0.0

@tonyo
Copy link
Contributor

tonyo commented May 23, 2023

@asottile-sentry could you clarify why defaulting to "0.0.0" or similar as a return value of getLatestTag() is not an acceptable fix here? Looks like getLatestTag is only used in one place at this point, so we can update it with less friction.

If some script hooks rely on the existence of the tag, I feel like we should just update those. We've hit this issue like 3+ (?) times already, and the quick start guide doesn't seem to help 😓

@asottile-sentry
Copy link
Member

could you clarify what you mean by "doesn't seem to help"? if the directions are followed everything works

0.0.0 breaks several things including changelog generation and the copy pasted version bumping scripts

@tonyo
Copy link
Contributor

tonyo commented May 23, 2023

could you clarify what you mean by "doesn't seem to help"?

People forget to check the instructions, miss that specific point, or think that they don't apply to their specific case. It just feels like something that can be fixed without too much effort and make people's lives easier, but I might be wrong about the "without too much effort" part, of course.

copy pasted version bumping scripts

Do you have some of those in mind? IIRC not that many scripts actually rely on the existing/valid previous version.

@asottile-sentry
Copy link
Member

at a glance:

$ all-repos-grep --repos '\$1' -- scripts/bump-version.sh
repos/getsentry/arroyo
repos/getsentry/chartcuterie
repos/getsentry/pytest-responses
repos/getsentry/raven-python
repos/getsentry/rb
repos/getsentry/responses
repos/getsentry/self-hosted
repos/getsentry/sentry
repos/getsentry/sentry-elixir
repos/getsentry/sentry-go
repos/getsentry/sentry-java
repos/getsentry/sentry-kotlin-multiplatform
repos/getsentry/sentry-native
repos/getsentry/sentry-php
repos/getsentry/sentry-python
repos/getsentry/sentry-rust
repos/getsentry/sentry-symfony
repos/getsentry/snuba
repos/getsentry/snuba-sdk
repos/getsentry/zeus-cli

@tonyo
Copy link
Contributor

tonyo commented May 23, 2023

Yep, looks like most of those scripts will accept an empty string or "0.0.0" as the first argument without any issues, and they don't use the old version for anything there, e.g.:

https://github.com/getsentry/arroyo/blob/main/scripts/bump-version.sh
https://github.com/getsentry/chartcuterie/blob/master/scripts/bump-version.sh
https://github.com/getsentry/pytest-responses/blob/main/scripts/bump-version.sh

@asottile-sentry
Copy link
Member

you've cherry picked the few that don't it seems -- most of them use it:

$ all-repos-grep '\$1' -- scripts/bump-version.sh
repos/getsentry/arroyo:scripts/bump-version.sh:    perl -i -pe "s/$1/$2/g" $3
repos/getsentry/chartcuterie:scripts/bump-version.sh:OLD_VERSION="$1"
repos/getsentry/pytest-responses:scripts/bump-version.sh:    sed -e "s/$1/$2/g" "$3" > "$3.tmp"  # -i is non-portable
repos/getsentry/raven-python:scripts/bump-version.sh:    perl -i -pe "s/$1/$2/g" $3
repos/getsentry/rb:scripts/bump-version.sh:OLD_VERSION="$1"
repos/getsentry/responses:scripts/bump-version.sh:    sed -e "s/$1/$2/g" "$3" > "$3.tmp"  # -i is non-portable
repos/getsentry/self-hosted:scripts/bump-version.sh:OLD_VERSION="$1"
repos/getsentry/sentry:scripts/bump-version.sh:OLD_VERSION="$1"
repos/getsentry/sentry-elixir:scripts/bump-version.sh:    perl -i -pe "s/$1/$2/g" $3
repos/getsentry/sentry-go:scripts/bump-version.sh:    perl -i -pe "s!$1!$2!g" $3
repos/getsentry/sentry-java:scripts/bump-version.sh:OLD_VERSION="$1"
repos/getsentry/sentry-kotlin-multiplatform:scripts/bump-version.sh:OLD_VERSION="$1"
repos/getsentry/sentry-native:scripts/bump-version.sh:OLD_VERSION="$1"
repos/getsentry/sentry-php:scripts/bump-version.sh:    perl -i -pe "s/$1/$2/g" $3
repos/getsentry/sentry-python:scripts/bump-version.sh:    perl -i -pe "s/$1/$2/g" $3
repos/getsentry/sentry-rust:scripts/bump-version.sh:perl -pi -e "s/^(sentry.*)?version = \".*?\"/\$1version = \"$NEW_VERSION\"/" sentry*/Cargo.toml
repos/getsentry/sentry-symfony:scripts/bump-version.sh:    perl -i -pe "s/$1/$2/g" $3
repos/getsentry/snuba:scripts/bump-version.sh:OLD_VERSION="$1"
repos/getsentry/snuba-sdk:scripts/bump-version.sh:    perl -i -pe "s/$1/$2/g" $3
repos/getsentry/zeus-cli:scripts/bump-version.sh:OLD_VERSION="$1"

@tonyo
Copy link
Contributor

tonyo commented May 23, 2023

What I mean by "use" is that $OLD_VERSION is utilized for anything non-trivial, and breaks the script if it's empty or points to an invalid version/tag.

Take this one from your list, for example: https://github.com/getsentry/sentry-native/blob/master/scripts/bump-version.sh

Yes, $OLD_VERSION is set, but only $NEW_VERSION is actually used for substitutions.

@tonyo
Copy link
Contributor

tonyo commented May 23, 2023

Sorry, that was bad example, but e.g. "${1}" in https://github.com/getsentry/snuba-sdk/blob/main/scripts/bump-version.sh is used inside a bash function, so it refers to a function argument, not to the script argument.

@BYK
Copy link
Member

BYK commented May 23, 2023

Looks like there are now 4 independent reports of this so it is clear that whatever we have in place does not work or protect people against this well-known failure mode.

My memory suggests that nothing in those scripts were relying on the $OLD_VERSION argument or env variable but it definitely needs validation after all this time.

Looks like if the script issue is resolved, it won't be too hard to default to the first commit for changelog generation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants