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

chadwhitacre commented 2 years ago

Steps to Reproduce

Start a new repo.
Commit a simple .craft.yml file.
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:

https://github.com/getsentry/craft/blob/2ff325e4d0ac9d4a8410b8107cede9144c2b91b6/src/utils/git.ts#L35-L38

See:

$ git describe --tags --abbrev=0
fatal: No names found, cannot describe anything.
$

kamilogorek commented 2 years ago

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 commented 2 years ago

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

kamilogorek commented 2 years ago

Quick workaround for posterity:

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

asottile-sentry commented 2 years ago

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

asottile-sentry commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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?

getsentry / craft