Replace upload-dif/upload-dsym with debug-files upload

[denrase]

Closes #194

• The way it is now, this would be a breaking change, as upload-dif/upload-dsym are removed. We could also leave them in, or call debug-files upload internally. -> Keep previous commands but document as deprecated.

[philipphofmann]

@denrase, please leave the public APIs in to make this a non-breaking change and forward the calls to the other methods. If possible, we should mark upload-dif/upload-dsym as deprecated and / or mention that in the README docs.

[denrase]

@philipphofmann @brustolin Any one of you up for a review? Thx! 🙇‍♂️

[philipphofmann]

Ups sorry @denrase. Will give you a review tomorrow.

[brustolin]

LGTM

[philipphofmann]

There is plenty of duplication in the code, which is kind of what we want 😄 as the actions are basically the same, but it would be great if we could reuse some code to get rid of duplication.

[philipphofmann]

m: I think it would be great to still test the deprecated way.

[philipphofmann]

m: The ConfigItems here are the same as here

sentry-fastlane-plugin/lib/fastlane/plugin/sentry/actions/sentry_upload_dif.rb

Lines 54 to 148 in 1b53185

	FastlaneCore::ConfigItem.new(key: :path,
	description: "Path or an array of paths to search recursively for symbol files",
	type: Array,
	optional: true),
	FastlaneCore::ConfigItem.new(key: :type,
	short_option: "-t",
	description: "Only consider debug information files of the given \
	type. By default, all types are considered",
	optional: true,
	verify_block: proc do |value|
	UI.user_error! "Invalid value '#{value}'" unless ['dsym', 'elf', 'breakpad', 'pdb', 'pe', 'sourcebundle', 'bcsymbolmap'].include? value
	end),
	FastlaneCore::ConfigItem.new(key: :no_unwind,
	description: "Do not scan for stack unwinding information. Specify \
	this flag for builds with disabled FPO, or when \
	stackwalking occurs on the device. This usually \
	excludes executables and dynamic libraries. They might \
	still be uploaded, if they contain additional \
	processable information (see other flags)",
	is_string: false,
	optional: true),
	FastlaneCore::ConfigItem.new(key: :no_debug,
	description: "Do not scan for debugging information. This will \
	usually exclude debug companion files. They might \
	still be uploaded, if they contain additional \
	processable information (see other flags)",
	conflicting_options: [:no_unwind],
	is_string: false,
	optional: true),
	FastlaneCore::ConfigItem.new(key: :no_sources,
	description: "Do not scan for source information. This will \
	usually exclude source bundle files. They might \
	still be uploaded, if they contain additional \
	processable information (see other flags)",
	is_string: false,
	optional: true),
	FastlaneCore::ConfigItem.new(key: :ids,
	description: "Search for specific debug identifiers",
	optional: true),
	FastlaneCore::ConfigItem.new(key: :require_all,
	description: "Errors if not all identifiers specified with --id could be found",
	is_string: false,
	optional: true),
	FastlaneCore::ConfigItem.new(key: :symbol_maps,
	description: "Optional path to BCSymbolMap files which are used to \
	resolve hidden symbols in dSYM files downloaded from \
	iTunes Connect. This requires the dsymutil tool to be \
	available",
	optional: true),
	FastlaneCore::ConfigItem.new(key: :derived_data,
	description: "Search for debug symbols in Xcode's derived data",
	is_string: false,
	optional: true),
	FastlaneCore::ConfigItem.new(key: :no_zips,
	description: "Do not search in ZIP files",
	is_string: false,
	optional: true),
	FastlaneCore::ConfigItem.new(key: :info_plist,
	description: "Optional path to the Info.plist.{n}We will try to find this \
	automatically if run from Xcode. Providing this information \
	will associate the debug symbols with a specific ITC application \
	and build in Sentry. Note that if you provide the plist \
	explicitly it must already be processed",
	optional: true),
	FastlaneCore::ConfigItem.new(key: :no_reprocessing,
	description: "Do not trigger reprocessing after uploading",
	is_string: false,
	optional: true),
	FastlaneCore::ConfigItem.new(key: :force_foreground,
	description: "Wait for the process to finish.{n}\
	By default, the upload process will detach and continue in the \
	background when triggered from Xcode. When an error happens, \
	a dialog is shown. If this parameter is passed Xcode will wait \
	for the process to finish before the build finishes and output \
	will be shown in the Xcode build output",
	is_string: false,
	optional: true),
	FastlaneCore::ConfigItem.new(key: :include_sources,
	description: "Include sources from the local file system and upload \
	them as source bundles",
	is_string: false,
	optional: true),
	FastlaneCore::ConfigItem.new(key: :wait,
	description: "Wait for the server to fully process uploaded files. Errors \
	can only be displayed if --wait is specified, but this will \
	significantly slow down the upload process",
	is_string: false,
	optional: true),
	FastlaneCore::ConfigItem.new(key: :upload_symbol_maps,
	description: "Upload any BCSymbolMap files found to allow Sentry to resolve \
	hidden symbols, e.g. when it downloads dSYMs directly from App \
	Store Connect or when you upload dSYMs without first resolving \
	the hidden symbols using --symbol-maps",
	is_string: false,
	optional: true)

It would be great to reuse some code here.

[philipphofmann]

m: That is the same as

sentry-fastlane-plugin/lib/fastlane/plugin/sentry/actions/sentry_upload_dif.rb

Lines 17 to 31 in 1b53185

	command.push('--type').push(params[:type]) unless params[:type].nil?
	command.push('--no-unwind') unless params[:no_unwind].nil?
	command.push('--no-debug') unless params[:no_debug].nil?
	command.push('--no-sources') unless params[:no_sources].nil?
	command.push('--ids').push(params[:ids]) unless params[:ids].nil?
	command.push('--require-all') unless params[:require_all].nil?
	command.push('--symbol-maps').push(params[:symbol_maps]) unless params[:symbol_maps].nil?
	command.push('--derived-data') unless params[:derived_data].nil?
	command.push('--no-zips') unless params[:no_zips].nil?
	command.push('--info-plist').push(params[:info_plist]) unless params[:info_plist].nil?
	command.push('--no-reprocessing') unless params[:no_reprocessing].nil?
	command.push('--force-foreground') unless params[:force_foreground].nil?
	command.push('--include-sources') unless params[:include_sources] != true
	command.push('--wait') unless params[:wait].nil?
	command.push('--upload-symbol-maps') unless params[:upload_symbol_maps].nil?

It would be great, if we could reuse some code.

[philipphofmann]

l: The spec is almost the same as sentry_upload_dif_spec. Again it would be great if we could reuse some code.

[denrase]

@philipphofmann So the files being the same is intentional. If we'd want to share code between the actions, we'd need to introduce common code, which will be removed anyway/. I also don't think changing the implementation of deprecated code makes much sense. WDYT?

[philipphofmann]

If we'd want to share code between the actions, we'd need to introduce common code, which will be removed anyway/. I also don't think changing the implementation of deprecated code makes much sense. WDYT?

Yes, but if we find some bugs, we're automatically fixing them for both, and I'm unsure when we will do the next major. If it's not complicated, I still would want to see the duplication gone. If it's complicated, then please merge, and we accept that little bit of duplication. Up to you.

[philipphofmann]

@denrase, can I merge the PR or do you still want to fix the duplication?

[denrase]

@philipphofmann Ah sry, let me merge in main and then we can merge. I'd leave the code now as-is, as it will be removed anyway. 🙇