Skip to content

fix(fixup): verify yaml key for skip: fix-patchelf and fix-machos#189

Merged
mxcl merged 6 commits intopkgxdev:mainfrom
tsirysndr:fix/rpath-fluentci.io
Oct 22, 2023
Merged

fix(fixup): verify yaml key for skip: fix-patchelf and fix-machos#189
mxcl merged 6 commits intopkgxdev:mainfrom
tsirysndr:fix/rpath-fluentci.io

Conversation

@tsirysndr
Copy link
Contributor

@jhheider
Copy link
Contributor

Definitely need to make all the formatting changes in a different branch if desired.

We should probably have a yaml key for skip-patchelf and skip-fix-machos.

@tsirysndr tsirysndr force-pushed the fix/rpath-fluentci.io branch from a815e0b to 475ebe2 Compare October 20, 2023 03:04
@tsirysndr tsirysndr changed the title fix(fix-elf): skip rpath fixes for fluentci.io fix(fixup): verify yaml key for skip-patchelf and skip-fix-machos Oct 20, 2023
@tsirysndr
Copy link
Contributor Author

I've added a check of the new yaml keys skip-patchelf and skip-fix-machos

@jhheider
Copy link
Contributor

I love it. Moves it to the yaml. Any concerns, @mxcl?

@tsirysndr tsirysndr force-pushed the fix/rpath-fluentci.io branch from 0d9af84 to 7fb63b7 Compare October 20, 2023 05:17
@mxcl
Copy link
Contributor

mxcl commented Oct 20, 2023

Why are we skipping? I prefer to fix the problem than work around it. If possible.

@jhheider
Copy link
Contributor

deno-built binaries can't have fix-elf.ts run on them, or it breaks the DATA segment look up, and they just become bloated deno binaries. it would let us generalize:

const skip_rpaths = [
"go.dev", // skipping because for some reason patchelf breaks the go binary resulting in the only output being: `Segmentation Fault`
"pkgx.sh", // this causes pkgx to pass -E/--version (and everything else?) directly to deno, making it _too_ much of a wrapper.
"render.com", // same as `pkgx.sh`
]
if (skip_rpaths.includes(installation.pkg.project)) {
console.info(`skipping rpath fixes for ${installation.pkg.project}`)
return
}

and give us a path when similar issues are found in the fix-machos.rb side.

will require build.skip-patchelf: true being added to these packages in pantry
@mxcl
Copy link
Contributor

mxcl commented Oct 20, 2023

Agreed, though I was looking for explanation for this pacakge? Does it use deno?

I think a more suitable syntax here would be skip: [fix-macho, fix-patchelf]

And we can do even better for v2 with some kind of composable build steps system.

@jhheider
Copy link
Contributor

yup, deno. this is our third, but surely far from our last. your skip string | string[] is more forward looking, and i like it.

@mxcl
Copy link
Contributor

mxcl commented Oct 20, 2023

I wonder if these other deno packages had the same problem as we did packaging for homebrew core?

@tsirysndr tsirysndr changed the title fix(fixup): verify yaml key for skip-patchelf and skip-fix-machos fix(fixup): verify yaml key for skip: patchelf and fix-machos Oct 20, 2023
@tsirysndr tsirysndr changed the title fix(fixup): verify yaml key for skip: patchelf and fix-machos fix(fixup): verify yaml key for skip: fix-patchelf and fix-machos Oct 20, 2023
Copy link
Contributor

@mxcl mxcl left a comment

Choose a reason for hiding this comment

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

lgtm! many thanks for a great patch

@jhheider
Copy link
Contributor

re: that test failure: I don't know where we're finding that pip tag. I think it must have been deleted, but the api is still serving it.

https://github.com/pypa/pip/tags

@mxcl mxcl merged commit efcd2fb into pkgxdev:main Oct 22, 2023
@mxcl
Copy link
Contributor

mxcl commented Oct 22, 2023

Crossing fingers that the build failure is nothing to do with this since it seems impossible it was.

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.

3 participants