detect.go does not parse values for .platform/env/BP_NATIVE_IMAGE correctly

[sebastiangraf]

I do not use pack but rely instead on paketobuildpacks/builder:tiny directly on my .gitlab-ci.yml. As a consequence, I write the variable like BP_NATIVE_IMAGE to .platform/env/BP_NATIVE_IMAGE and invoke cnb/lifecycle/creator -app=. -platform=$HOME/.platform in the container itself.

The native buildpack throws an error:

Paketo Buildpack for Native Image 5.8.0
  invalid value '1
  ' for key 'BP_NATIVE_IMAGE': expected one of [1, t, T, TRUE, true, True, 0, f, F, FALSE, false, False]

Searching for the string leads me to this buildpack. For the java-buildpack (where I set the BP_JVM_VERSION the same way), it works fine.

Expected Behavior

Setting the BP-*-variables should work for this build pack as denoted in https://github.com/buildpacks/spec/blob/main/platform.md#user-provided-variables

Executing the following commands in paketobuildpacks/builder:tiny should work and generate a native java application.

mkdir -p $HOME/.platform/env
echo "1" > $HOME/.platform/env/BP_NATIVE_IMAGE
echo "17" > $HOME/.platform/env/BP_JVM_VERSION
/cnb/lifecycle/creator -app=. -platform=$HOME/.platform image

Current Behavior

The commands above fail. I included some debug-code since the snippet is running in gitlab-ci to track the issue down:

$ cat $HOME/.platform/env/BP_NATIVE_IMAGE || true
cat: /home/cnb/.platform/env/BP_NATIVE_IMAGE: No such file or directory
$ rm $HOME/.platform/env/BP_NATIVE_IMAGE || true
$ echo "1" > $HOME/.platform/env/BP_NATIVE_IMAGE
rm: cannot remove '/home/cnb/.platform/env/BP_NATIVE_IMAGE': No such file or directory
$ cat $HOME/.platform/env/BP_NATIVE_IMAGE || true
1
$ echo "17" > $HOME/.platform/env/BP_JVM_VERSION
$ /cnb/lifecycle/creator -app=. -platform=$HOME/.platform $CI_REGISTRY_IMAGE:$CI_COMMIT_REF_SLUG-native
Warning: Not restoring or caching layer data, no cache flag specified.
===> DETECTING
======== Output: paketo-buildpacks/native-image@5.8.0 ========
Paketo Buildpack for Native Image 5.8.0
  invalid value '1
  ' for key 'BP_NATIVE_IMAGE': expected one of [1, t, T, TRUE, true, True, 0, f, F, FALSE, false, False]

Afterwards a jar-application is build without nativeness.

I tried multiple variants: 1, true, etc.

Possible Solution

I guess it is a bug in the detect.go...in the last commits some test cases where removed...but this is a guess.

Steps to Reproduce

1. Start paketobuildpacks/builder:tiny and enter the container: docker run --rm -it -v "$PWD":/usr/src/app -w /usr/src/app paketobuildpacks/builder:base bash
2. Set the files for the platform:

   mkdir -p $HOME/.platform/env
   echo "1" > $HOME/.platform/env/BP_NATIVE_IMAGE
   echo "17" > $HOME/.platform/env/BP_JVM_VERSION

3. Execute the build process and see the error

   
   cnb@bd1dc0691ed0:/usr/src/app$ /cnb/lifecycle/creator -app=. -platform=$HOME/.platform image
   Warning: Not restoring or caching layer data, no cache flag specified.
   ===> DETECTING
   ======== Output: paketo-buildpacks/native-image@5.8.0 ========

Paketo Buildpack for Native Image 5.8.0 invalid value '1 ' for key 'BP_NATIVE_IMAGE': expected one of [1, t, T, TRUE, true, True, 0, f, F, FALSE, false, False]

## Motivations
Letting buildpacks generate native java-image and run in gitlab-ci without autodevops-magic via the builder-image directly.

[dmikusa]

This is a bit of a guess, but it looks like there's an EOL/line break character in there. Again, just a guess, but the way Go parses the value into a bool is probably not compatible with that.

Can you try using printf or echo -n? Basically, something that doesn't write an EOL after the value.

[sebastiangraf]

Good guess: both commands work. Funny although that the java buildpack is recognizing the 17 with the line break.

Thanks a lot!

[dmikusa]

Excellent. I've tagged this as a bug. We should be able to just trim the string before parsing it to remove the EOL characters. If anyone is looking for a good first contribution, this would be one.

[anthonydahanne]

hello @sebastiangraf ! Could you please share a sample .gitlab-ci.yml in this ticket?

It's not clear to me how environment variables are passed to Gitlab AutoDevops.

Thank you!

[dmikusa]

@anthonydahanne this is where we parse the BP_NATIVE_IMAGE env variable. The value that gets parsed by strconv.ParseBool, which doesn't seem to like the EOL character.

A couple of notes on this:

1. The native-image buildpack should be using https://github.com/paketo-buildpacks/libpak/blob/main/sherpa/env_var.go#L57-L69. We added this a while back and have gradually tried to move buildpacks to use it so we get consistent parsing of env variables to booleans.
2. In libpak, we should probably do a strings.Trim on s before it goes into ParseBool. Just in case there's unnecessary whitespace in the env variable. I think that will fix the issue.

[anthonydahanne]

@dmikusa thanks for your input; I'm still curious to see the Gitlab integration in action though; I shall try it out myself sooner or later! (curious to see / test the different buildpacks integration)

Wrt to your suggestions:

2. yes, that makes sense , we could probably add to ResolvBool in sherpa/env_var.go something like:

    t := strings.Trim(s, "\r \n\t")
    p, err := strconv.ParseBool(t)

but....

1. We would change the behavior by using ResolvBool from sherpa/env_var.go - because this function does not make the difference between "user set to false" and "user input was neither true nor false" ; since the parse err is converted into false. If we had already the same behavior, @sebastiangraf would not have been able to provide insightful log like he did; so I believe this distinction to be important.

I suggest in libpak we add another ResolveBoolErr signature so that the developer can decide whether or not we want the parse error; in our case (native-image property parsing), we'd use this function.

// ResolveBoolErr resolves a boolean value for a configuration option.
// Returns true, nil for 1, t, T, TRUE, true, True.
// Returns false, nil for all other values or unset.
// Returns false, error is the value could not be parsed into a bool
func ResolveBoolErr(name string) (bool, error) {
    s, ok := os.LookupEnv(name)
    if !ok {
        return false, nil
    }

    t := strings.Trim(s, "\r \n\t")
    p, err := strconv.ParseBool(t)
    if err != nil {
        return false, fmt.Errorf(
            "invalid value '%s' for key '%s': expected one of [1, t, T, TRUE, true, True, 0, f, F, FALSE, false, False]",
            s,
            name,
        )
    }

    return p, nil
}

// ResolveBool resolves a boolean value for a configuration option. Returns true for 1, t, T, TRUE, true, True. Returns
// false for all other values or unset.
func ResolveBool(name string) bool {
    resolveBool, _ := ResolveBoolErr(name)
    return resolveBool
}

Let me know what you think! Thanks!

[dmikusa]

t := strings.Trim(s, "\r \n\t")

Oops. I meant strings.TrimSpace https://pkg.go.dev/strings#TrimSpace

I suggest in libpak we add another ResolveBoolErr signature so that the developer can decide whether or not we want the parse error; in our case (native-image property parsing), we'd use this function.

I can't argue that it was helpful, it was really nice to have the error message. It would have been tricky to diagnose with the current ResolveBool implementation. I'm always in favor of good debugability.

I'm OK with adding a ResolveBoolErr as you suggested. It would be really helpful if we could log the error for the ResolveBool method, but I don't think we have a logger available.

We have to start thinking about libpak 2 soon. The libcnb 2 should hopefully be released soon, and we'll need libpak 2 for parity. That'll be a chance to revisit some of this, perhaps having a standard logger available or just having ResolveBool return an error always (you can ignore it if you want). Anyway, I'm rambling now. Your suggestions look great!