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

Allow same globs in multiple slices #62

Closed
wants to merge 1 commit into from

Conversation

woky
Copy link
Contributor

@woky woky commented May 17, 2023

There's no reason why the following slices shouldn't be accepted when
the globs are identical:

package: openjdk-8-jre-headless
slices:
  abc:
    contents:
      /usr/lib/jvm/java-8-openjdk-*/jre/lib/*/libnpt.so:
  bcd:
    contents:
      /usr/lib/jvm/java-8-openjdk-*/jre/lib/*/libnpt.so:

Drop this restriction.

  • Have you signed the CLA?

@woky woky force-pushed the allow-same-glob branch 2 times, most recently from 204bf0f to c5bfbc5 Compare May 17, 2023 12:20
There's no reason why the following slices shouldn't be accepted when
the globs are identical:

    package: openjdk-8-jre-headless
    slices:
      abc:
        contents:
          /usr/lib/jvm/java-8-openjdk-*/jre/lib/*/libnpt.so:
      bcd:
        contents:
          /usr/lib/jvm/java-8-openjdk-*/jre/lib/*/libnpt.so:

Drop this restriction.
@cjdcordeiro
Copy link
Collaborator

for additional context:

Scenario A

package: openjdk-8-jre-headless
slices:
  abc:
    contents:
      /usr/lib/jvm/java-8-openjdk-*/jre/lib/*/libnpt.so:
  bcd:
    contents:
      /usr/lib/jvm/java-8-openjdk-*/jre/lib/*/libnpt.so:
$ chisel cut --release $PWD --root scenarioA openjdk-8-jre-headless_abc openjdk-8-jre-headless_bcd
(...)
error: cannot extract from package "openjdk-8-jre-headless": when using wildcards source and target paths must match: /usr/lib/jvm/java-8-openjdk-/jre/lib//libnpt.so

Scenario B

package: openjdk-8-jre-headless
slices:
  abc:
    contents:
      /usr/lib/jvm/java-8-openjdk-*/jre/lib/*/libnpt.so:
  bcd:
    contents:
      /usr/lib/jvm/java-8-openjdk-amd64/jre/lib/amd64/libnpt.so:
$ chisel cut --release $PWD --root scenarioA openjdk-8-jre-headless_abc openjdk-8-jre-headless_bcd
(...)
error: cannot extract from package "openjdk-8-jre-headless": no content at /usr/lib/jvm/java-8-openjdk-am64/jre/lib/amd64/libnpt.so

Scenario C

If both paths are explicit (i.e. without globs), it works.

Copy link
Collaborator

@cjdcordeiro cjdcordeiro left a comment

Choose a reason for hiding this comment

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

@woky thanks for looking into this!

Just a couple of questions:

  1. does this change only apply to slices within the same package? I.e. that constraint should still be valid when that path is in different packages...
  2. can you verify that the solution also works for scenario B in Allow same globs in multiple slices #62 (comment)?
    • while at it, can you please add another test to reflect that scenario?

Copy link
Member

@rebornplusplus rebornplusplus left a comment

Choose a reason for hiding this comment

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

Good initiative!

I am a bit confused if this PR implies that the globs must be identical. If the globs can be the same without being identical, i.e. matches the same paths without being the identical copy of another glob char-by-char, then I am afraid the changes won't be enough.

For example, as Cris mentioned in his comment (#62 (comment)), scenario B does not work yet. But that is a glob in one slice and an explicit path in another. I had the following SDF and it failed as well (logs attached).

package: openjdk-8-jre-headless
slices:
  abc:
    contents:
      /usr/lib/jvm/java-8-openjdk-*/jre/lib/*/libnpt.so:
  bcd:
    contents:
      /usr/lib/jvm/java-8-openjdk-*/jre/lib/*/libn*t.so:

Failure log:

error: cannot extract from package "openjdk-8-jre-headless": no content at /usr/lib/jvm/java-8-openjdk-*/jre/lib/*/libnpt.so

Notice how the globs are the same in the context, but not identical. I think this is because pendingPaths is not empty at the end. The reason is that for one explicit path in the package, only one of the globs is cleared off from the checklist. Maybe we should modify shouldExtract to return the list of globs/paths it matches instead of just the one.

With all that being said, if the globs must be identical char-by-char instead of being same, then this PR seems OK to me. 👍

@woky
Copy link
Contributor Author

woky commented May 18, 2023

You are right @rebornplusplus. Thanks for the explanation. Converting to draft for now.

@woky
Copy link
Contributor Author

woky commented Jun 22, 2023

Closing as this is not really a fix. It's going to be easier to fix with #74 merged.

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.

4 participants