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

feat: follow symlinks once when counting classes #452

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

peterhaochen47
Copy link

@peterhaochen47 peterhaochen47 commented Jan 6, 2025

  • follow once for now to avoid potential infinite loops, in case there is circular linking
  • problem statement: we have a use case where the final class count needs to include the classes in the targets of the symlinks

Summary

  • follow symlinks once when counting classes (but only once to avoid potential infinite loops, in case there is circular linking)

Use Cases

  • we have a use case where the final class count needs to include the classes in the targets of the symlinks

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • (n/a) I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

- follow once for now to avoid potential infinite loops,
in case there is circular linking
- problem statement: we have a use case where the final
class count needs to include the classes in the targets
of the symlinks
@peterhaochen47 peterhaochen47 requested a review from a team as a code owner January 6, 2025 22:08
@dmikusa
Copy link
Contributor

dmikusa commented Jan 7, 2025

we have a use case where the final class count needs to include the classes in the targets of the symlinks

Can you expand on this? Is there a scenario you can walk me through to explain how that's possible?

Also, as a side note/FYI. We are in the middle of building out v2 of our libraries and new feature work is slated to go into v2 releases, not v1 releases. This may result in a longer than normal delay before new feature work can be incorporated.

@peterhaochen47
Copy link
Author

peterhaochen47 commented Jan 7, 2025

Can you expand on this? Is there a scenario you can walk me through to explain how that's possible?

Yes. Essentially, we are trying to make the bellsoft-liberica buildpack work within the context of a different directory structure, as in different from the directory structure of an oci image (which the buildpack today assumes).

To bridge those discrepancies in directory structures, we created various symlinks in our context, and the buildpack will follow those symlinks and arrive at the correct targets, except for the helper being modified in this PR.

@dmikusa
Copy link
Contributor

dmikusa commented Jan 7, 2025

Can you expand on this? Is there a scenario you can walk me through to explain how that's possible?

Yes. Essentially, we are trying to make the bellsoft-liberica buildpack work within the context of a different directory structure, as in different from the directory structure of an oci image (which the buildpack today assumes).

Not sure I follow, sorry. What directory structure are you referring to? The contents of an OCI image can literally be from nothing to anything. Are you referring to some of the buildpack directory structures? or Java/Spring directory structures?

@peterhaochen47
Copy link
Author

peterhaochen47 commented Jan 7, 2025

Can you expand on this? Is there a scenario you can walk me through to explain how that's possible?

Yes. Essentially, we are trying to make the bellsoft-liberica buildpack work within the context of a different directory structure, as in different from the directory structure of an oci image (which the buildpack today assumes).

Not sure I follow, sorry. What directory structure are you referring to? The contents of an OCI image can literally be from nothing to anything. Are you referring to some of the buildpack directory structures? or Java/Spring directory structures?

So reading the various symlinks / env var defaults in the buildpack today, it's assuming a directory structure like this:

\workspace
\layers
\cnb
\....other-things

Which is the directory structure of a docker container run with an OCI image built by the buildpack. An example of such an assumption is that I can see the following symlinks inside of the said docker container's /cnb/process directory:

executable-jar -> /cnb/lifecycle/launcher
web -> /cnb/lifecycle/launcher
task -> /cnb/lifecycle/launcher

Where the targets of those links are absolute paths, assuming that there is a /cnb/....

So we are trying to make the buildpack work (including making those symlinks work) on a different directory structure (where there is certain limitation that does not allow us to structure it like the docker container). To bridge this gap, we created various symlinks, something like:

\workspace -> [symlinked to where we actually put the workspace dir]
\layers -> [symlinked to where we actually put the layers dir]
\cnb -> [symlinked to where we actually put the cnb dir]
\....other-things

This mostly works, with the exception of the class counting.

@dmikusa
Copy link
Contributor

dmikusa commented Jan 7, 2025

I'm hesitant here because a lot of what you're describing sounds like it could/should be done in other ways. Most of those locations you've mentioned can be defined by a platform, and it sounds like you're making a custom platform, so rather than use symlinks to bend things around, the platform should arguably be invoking the lifecycle commands and pointing directly to where it is putting things.

For example, there are flags for changing the layers, workspace, and platform directories. I'm not totally certain about /cnb, but I don't think that's part of the issue you see here either, so it may not matter.

Have you looked at those options?

@peterhaochen47
Copy link
Author

peterhaochen47 commented Jan 7, 2025

I'm hesitant here because a lot of what you're describing sounds like it could/should be done in other ways. Most of those locations you've mentioned can be defined by a platform, and it sounds like you're making a custom platform, so rather than use symlinks to bend things around, the platform should arguably be invoking the lifecycle commands and pointing directly to where it is putting things.

For example, there are flags for changing the layers, workspace, and platform directories. I'm not totally certain about /cnb, but I don't think that's part of the issue you see here either, so it may not matter.

Have you looked at those options?

Yeah, I understand your hesitancy. We are also just exploring various options and have not decided on the long term path. But I can explain why we are trying this currently. So far, we can indeed set some of those vars to tell the buildpack what our true directories are:

CNB_LAYERS_DIR "/where-we-actually-put/layers"
CNB_APP_DIR "/where-we-actually-put/workspace"

This works mostly, with these exceptions:

  • the class counting code is not looking at CNB_APP_DIR; instead it's looking at BPI_APPLICATION_PATH (which looks like an internal var based on its name and the appearance that it's undocumented?) whose default is /workspace. And yes, if we overwrite BPI_APPLICATION_PATH, it would work for us. But we were hoping that our "platform" would work for any buildpacks (not just java ones) and not have to do anything specific to a certain buildpack, hence we were trying the symlink idea.
  • and also the numerous symlinks (e.g. executable-jar -> /cnb/lifecycle/launcher mentioned above) are hardcoded and are not following CNB_LAYERS_DIR and CNB_APP_DIR. And we would have had to edit all those symlinks, and we were trying to avoid that.

@dmikusa
Copy link
Contributor

dmikusa commented Jan 7, 2025

the class counting code is not looking at CNB_APP_DIR; instead it's looking at BPI_APPLICATION_PATH (which looks like an internal var based on its name and the appearance that it's undocumented?) whose default is /workspace. And yes, if we overwrite BPI_APPLICATION_PATH, it would work for us. But we were hoping that our "platform" would work for any buildpacks (not just java ones) and not have to do anything specific to a certain buildpack, hence we were trying the symlink idea.

That sounds like a bug to me. The buildpack shouldn't be assuming /workspace anywhere. If you open up an issue for that, we can definitely fix that one.

and also the numerous symlinks (e.g. executable-jar -> /cnb/lifecycle/launcher mentioned above) are hardcoded and are not following CNB_LAYERS_DIR and CNB_APP_DIR. And we would have had to edit all those symlinks, and we were trying to avoid that.

I don't think that's us, I think that's upstream. You might try reaching out on https://github.com/buildpacks/lifecycle or on Buildpacks Slack and asking about /cnb and if there are ways to change that. Maybe there is something? It seems a little arbitrary to force that path, ultimately the entrypoint is going to invoke launcher, it doesn't matter if it's at /cnb/... or /foo/cnb/....

@peterhaochen47
Copy link
Author

peterhaochen47 commented Jan 8, 2025

That sounds like a bug to me. The buildpack shouldn't be assuming /workspace anywhere. If you open up an issue for that, we can definitely fix that one.

Actually, I'll look into this more before saying it's a bug.

I neglected to mention that our intention is to build the app oci image with the buildpack for the normal docker platform, aka with these settings during build:

  "CNB_LAYERS_DIR=/layers",
  "CNB_APP_DIR=/workspace",
  "CNB_PLATFORM_API=0.9",
  "CNB_DEPRECATION_MODE=quiet"

And then have the resulting image not only be runnable with docker but also (with some modifications) be runnable on our platform. So maybe the buildpack would have correctly set BPI_APPLICATION_PATH to "/where-we-actually-put/workspace" if we had set CNB_APP_DIR="/where-we-actually-put/workspace" during build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants