Skip to content

Conversation

@startergo
Copy link
Contributor

Replace xattr command with find to remove quarantine attribute.

Replace xattr command with find to remove quarantine attribute.
Copilot AI review requested due to automatic review settings August 25, 2025 23:16
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR modifies the quarantine attribute removal method by replacing the xattr -drs command with a find command that executes xattr -d on each file individually.

  • Replaces xattr -drs with find ... -exec xattr -d for quarantine attribute removal
  • Changes error handling from strict exit on failure to silently continuing with || true
  • Removes the original error status code reporting

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

createinstalliso Outdated
echo "Copying installer application..."
ditto "${original_installer_application_path}" "${modified_installer_application_path}" >/dev/null || exit_with_status 220 "Failed to copy installer application."
xattr -drs "com.apple.quarantine" "${modified_installer_application_path}" >/dev/null || exit_with_status 219 "Failed to remove extended attribute com.apple.quarantine."
find "${modified_installer_application_path}" -exec xattr -d com.apple.quarantine {} \; 2>/dev/null || true # "${modified_installer_application_path}" >/dev/null || exit_with_status 219 "Failed to remove extended attribute com.apple.quarantine."
Copy link

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

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

The error handling has been changed from providing a meaningful error message (exit code 219 with 'Failed to remove extended attribute com.apple.quarantine') to silently ignoring all failures with || true. This removes important error reporting that could help diagnose issues.

Suggested change
find "${modified_installer_application_path}" -exec xattr -d com.apple.quarantine {} \; 2>/dev/null || true # "${modified_installer_application_path}" >/dev/null || exit_with_status 219 "Failed to remove extended attribute com.apple.quarantine."
find "${modified_installer_application_path}" -exec xattr -d com.apple.quarantine {} \; 2>/dev/null || exit_with_status 219 "Failed to remove extended attribute com.apple.quarantine."

Copilot uses AI. Check for mistakes.
createinstalliso Outdated
echo "Copying installer application..."
ditto "${original_installer_application_path}" "${modified_installer_application_path}" >/dev/null || exit_with_status 220 "Failed to copy installer application."
xattr -drs "com.apple.quarantine" "${modified_installer_application_path}" >/dev/null || exit_with_status 219 "Failed to remove extended attribute com.apple.quarantine."
find "${modified_installer_application_path}" -exec xattr -d com.apple.quarantine {} \; 2>/dev/null || true # "${modified_installer_application_path}" >/dev/null || exit_with_status 219 "Failed to remove extended attribute com.apple.quarantine."
Copy link

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

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

The find command with -exec will spawn a separate xattr process for each file found, which is less efficient than the original xattr -drs command that could process multiple files in a single invocation. Consider using find ... -print0 | xargs -0 xattr -d for better performance with many files.

Suggested change
find "${modified_installer_application_path}" -exec xattr -d com.apple.quarantine {} \; 2>/dev/null || true # "${modified_installer_application_path}" >/dev/null || exit_with_status 219 "Failed to remove extended attribute com.apple.quarantine."
find "${modified_installer_application_path}" -print0 | xargs -0 xattr -d com.apple.quarantine 2>/dev/null || true # "${modified_installer_application_path}" >/dev/null || exit_with_status 219 "Failed to remove extended attribute com.apple.quarantine."

Copilot uses AI. Check for mistakes.
startergo and others added 2 commits August 25, 2025 19:29
The find command with -exec will spawn a separate xattr process for each file found, which is less efficient than the original xattr -drs command that could process multiple files in a single invocation.
using `find ... -print0 | xargs -0 xattr -d` for better performance with many files.
@BITespresso
Copy link
Owner

Hi @startergo,

thanks for your PR. Could you briefly explain why you suggested this change?

As far as I can see, you

a) want the command to remove the quarantine flag to never fail
b) want execute it for each file separately

This makes sense if the command fails for certain files but not for others. It also means that, for the files where it fails, the quarantine flag does not break anything.

Can you provide an example of when this situation occurred? I'd be interested to know which files fail to have the quarantine flag removed, and where you downloaded the installer application from.

The reason for asking is, that I want to investigate why this happens and why your PR is required.

Thanks!

@startergo
Copy link
Contributor Author

startergo commented Sep 7, 2025

It was failing to build on the silicon Mac. It would just exit at that line. Now it works as designed.

xattr -drs "com.apple.quarantine"  
option -r not recognized

usage: xattr [-slz] file [file ...]
       xattr -p [-slz] attr_name file [file ...]
       xattr -w [-sz] attr_name attr_value file [file ...]
       xattr -d [-s] attr_name file [file ...]
       xattr -c [-s] file [file ...]

The first form lists the names of all xattrs on the given file(s).
The second form (-p) prints the value of the xattr attr_name.
The third form (-w) sets the value of the xattr attr_name to attr_value.
The fourth form (-d) deletes the xattr attr_name.
The fifth form (-c) deletes (clears) all xattrs.

options:
  -h: print this help
  -s: act on symbolic links themselves rather than their targets
  -l: print long format (attr_name: attr_value)
  -z: compress or decompress (if compressed) attribute value in zip format

I guess somehow python overrode xattr in /usr/bin/xattr:

which -a xattr 
/Library/ManagedFrameworks/Python/Python3.framework/Versions/3.12/bin/xattr
/usr/bin/xattr

and :

which xattr
/Library/ManagedFrameworks/Python/Python3.framework/Versions/3.12/bin/xattr

But the python's xattr does not include -r option.

So in this respect a fix could be providing a simple absolute path for xattr rather than relative.

Add absolute path for the `xattr` command to use the Apple provided `xattr` command as the python version of `xattr` does not recognize `-r` (recursive) option.
@BITespresso
Copy link
Owner

@startergo Thanks for the update and the explanation behind the issue. So this falls into the same category as the issue here: #15

Therefore, I'd rather make an update that includes all external commands called from the script rather than fixing this command by command. I put that on my list for the next update.

Thanks for your contribution and explanation.

Michael

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