Skip to content

feature/demrum 1571 dsym extensions #102

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Mar 13, 2025
Merged

feature/demrum 1571 dsym extensions #102

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
74fec5c
DEMRUM-1571: returning non-zero error code in failure cases
carlosmcevilly Mar 6, 2025
f5c84a1
DEMRUM-1571: Allow upload of .dSYM/ and .dSYM.zip and .dSYMs.zip, not…
carlosmcevilly Mar 6, 2025
2076a5c
DEMRUM-1571: improve matching of dSYMs extensions
carlosmcevilly Mar 7, 2025
bd0d542
DEMRUM-1571: improve error message
carlosmcevilly Mar 7, 2025
e967b8b
DEMRUM-1571: restore missing header
carlosmcevilly Mar 7, 2025
a060ba2
DEMRUM-1571: use iOSCommand.error()
carlosmcevilly Mar 11, 2025
6173c7c
DEMRUM-1571: use logger to output info on what is being zipped
carlosmcevilly Mar 13, 2025
0b35284
DEMRUM-1571: move iOSdSYMUtils into dsyms directory
carlosmcevilly Mar 13, 2025
12ab4b9
DEMRUM-1571: Update --directory to --path and fix related UX and comm…
carlosmcevilly Mar 13, 2025
2e01418
DEMRUM-1571: suppress display of currently unused -h and --help for now
carlosmcevilly Mar 13, 2025
2f35d1b
Merge branch 'main' into feature/DEMRUM-1571-dsym-extensions
carlosmcevilly Mar 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions src/commands/ios.ts
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,7 @@ iOSCommand
.action(async (options: UploadCommandOptions) => {
const token = options.token || process.env.O11Y_TOKEN;
if (!token) {
console.error('Error: API access token is required.');
process.exit(1);
iOSCommand.error('Error: API access token is required.');
}
options.token = token;

Expand Down Expand Up @@ -172,6 +171,7 @@ iOSCommand
logger.error(error);
logger.error(unableToUploadMessage);
}
iOSCommand.error('');
}
}
cleanupTemporaryZips(uploadPath);
Expand All @@ -182,8 +182,8 @@ iOSCommand
logger.debug(error.originalError);
} else {
logger.error('An unexpected error occurred:', error);
throw error;
}
iOSCommand.error('');
}
});

Expand All @@ -205,8 +205,7 @@ iOSCommand
.action(async (options: ListCommandOptions) => {
const token = options.token || process.env.O11Y_TOKEN;
if (!token) {
console.error('Error: API access token is required.');
process.exit(1);
iOSCommand.error('Error: API access token is required.');
}
options.token = token;

Expand Down Expand Up @@ -238,7 +237,6 @@ iOSCommand
} else {
logger.error('Failed to fetch the list of uploaded files:', String(error));
}
throw error;
iOSCommand.error('');
}
});

152 changes: 92 additions & 60 deletions src/utils/iOSdSYMUtils.ts
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

import { tmpdir } from 'os';
import { execSync } from 'child_process';
import { join, resolve } from 'path';
import { join, resolve, basename, dirname } from 'path';
import { copyFileSync, mkdtempSync, readdirSync, rmSync, statSync } from 'fs';
import { UserFriendlyError, throwAsUserFriendlyErrnoException } from '../utils/userFriendlyErrors';

Expand All @@ -26,21 +26,101 @@ import { UserFriendlyError, throwAsUserFriendlyErrnoException } from '../utils/u
export function validateDSYMsPath(dsymsPath: string): string {
const absPath = resolve(dsymsPath);

if (!absPath.endsWith('dSYMs')) {
throw new UserFriendlyError(null, `Invalid input: Expected a path ending in 'dSYMs'.`);
if (absPath.endsWith('/dSYMs') || absPath === 'dSYMs') {
try {
const stats = statSync(absPath);
if (!stats.isDirectory()) {
throw new UserFriendlyError(null, `Invalid input: Expected a 'dSYMs/' directory but got a file.`);
}
} catch (err) {
throwAsUserFriendlyErrnoException(err, {
ENOENT: `Path not found: Ensure the provided directory exists before re-running.`,
});
}
return absPath;
}

try {
const stats = statSync(absPath);
if (!stats.isDirectory()) {
throw new UserFriendlyError(null, `Invalid input: Expected a 'dSYMs/' directory but got a file.`);
if (absPath.endsWith('.dSYM.zip') || absPath.endsWith('.dSYMs.zip')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The option is named --directory, but it can take a file, too.

--path would work better, is there any other alternative? Other commands in CLI don't use --path option right now, they only use --directory or --file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah good point... --path sounds good to me unless we want the extra sort of runtime type check of having two different ones. I could go either way, have both --file and --directory with checks, or just --path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just --path is going to be more simple to present in the usage message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

try {
const stats = statSync(absPath);
if (!stats.isFile()) {
throw new UserFriendlyError(null, `Invalid input: Expected a '.dSYM.zip' or '.dSYMs.zip' file.`);
}
} catch (err) {
throwAsUserFriendlyErrnoException(err, {
ENOENT: `File not found: Ensure the provided file [${absPath}] exists before re-running.`,
});
}
} catch (err) {
throwAsUserFriendlyErrnoException(err, {
ENOENT: `Path not found: Ensure the provided directory exists before re-running.`,
});
return absPath;
}
return absPath;

if (absPath.endsWith('.dSYM')) {
try {
const stats = statSync(absPath);
if (!stats.isDirectory()) {
throw new UserFriendlyError(null, `Invalid input: Expected a '.dSYM' directory but got a file.`);
}
} catch (err) {
throwAsUserFriendlyErrnoException(err, {
ENOENT: `Directory not found: Ensure the provided directory exists before re-running.`,
});
}
return absPath;
}

throw new UserFriendlyError(null, `Invalid input: Expected a path named 'dSYMs' or ending in '.dSYM', '.dSYMs.zip', or '.dSYM.zip'.`);
}

/**
* Given a dSYMs path, process the input as per the specified format and return
* the zipped files or copied files as necessary.
**/
export function getZippedDSYMs(dsymsPath: string): { zipFiles: string[], uploadPath: string } {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd consider adding debug logging in a function like this, to provide some level of visibility into what the tool is zipping together based on the command options.

I added logging to a similar command for sourcemaps that traverses file system and operates on some specific files under some specific conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I've added it with logger.info() at the moment which gives us at least the imports and access to the logger object to enhance it more with logger.debug() in case of errors -- it's only used in one spot but please let me know if it maps to what you had in mind.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's along the lines of what I was thinking: log the results of the directory scanning so we can clearly show what files are getting touched. Since in cases like this (and sourcemaps inject), the user is just providing us a directory that we scan. But logs would make it very clear which specific files or folders inside of it we are actually handling.

I'd suggest logging in the next for-loop, too (line 92), for similar reasons. That way we could clearly see the other files that we found and are operating on. Not sure if logger.info() or logger.debug() is more appropriate.

Originally, I was thinking a debug log would be nice in the other if-else branches, too. But if they are handling a single file/folder that was passed in as the --path, then perhaps it's not needed. It's clear from the --path what the tool will be using.

And info level is fine too, if it's a detail that we would want to always show to the user.

const absPath = validateDSYMsPath(dsymsPath);

// Create a unique system temp directory for storing zip files
const uploadPath = mkdtempSync(join(tmpdir(), 'splunk_dSYMs_upload_'));

if (absPath.endsWith('dSYMs')) {
const { dSYMDirs, dSYMZipFiles } = scanDSYMsDirectory(absPath);
const results: string[] = [];

for (const dSYMDir of dSYMDirs) {
results.push(zipDSYMDirectory(absPath, dSYMDir, uploadPath));
}

for (const zipFile of dSYMZipFiles) {
const srcPath = join(absPath, zipFile);
const destPath = join(uploadPath, zipFile);
try {
copyFileSync(srcPath, destPath);
} catch (err) {
throwAsUserFriendlyErrnoException(err, {
ENOENT: `Failed to copy ${srcPath} to ${destPath}. Please ensure the file exists and is not in use.`,
EACCES: `Permission denied while copying ${srcPath}. Please check your access rights.`,
});
}
results.push(destPath);
}

return { zipFiles: results, uploadPath };
} else if (absPath.endsWith('.dSYM.zip') || absPath.endsWith('.dSYMs.zip')) {
const destPath = join(uploadPath, basename(absPath));
try {
copyFileSync(absPath, destPath);
} catch (err) {
throwAsUserFriendlyErrnoException(err, {
ENOENT: `Failed to copy ${absPath} to ${destPath}. Please ensure the file exists and is not in use.`,
EACCES: `Permission denied while copying ${absPath}. Please check your access rights.`,
});
}
return { zipFiles: [destPath], uploadPath };
} else if (absPath.endsWith('.dSYM')) {
const zipPath = zipDSYMDirectory(dirname(absPath), basename(absPath), uploadPath);
return { zipFiles: [zipPath], uploadPath };
}

throw new UserFriendlyError(null, `Unexpected error with the provided input path.`);
}

/**
Expand Down Expand Up @@ -100,51 +180,3 @@ export function cleanupTemporaryZips(uploadPath: string): void {
console.warn(`Warning: Failed to remove temporary directory '${uploadPath}'.`, err);
}
}

/**
* Given a dSYMs/ directory path, visit the contents of the directory and gather
* zipped copies of all the .dSYM/ directories it contains, including .dSYM/
* directories that were already zipped before we arrived. If both a .dSYM/ and
* its corresponding .dSYM.zip file exist, make a fresh .zip; if only the .zip
* exists, accept the .zip file. Put files (or, in the case of existing .zips,
* copies of them) in a temp dir `uploadPath`, then return the full list `zipFiles`
* of all .zip files placed there, along with the path to the temp dir itself.
**/
export function getZippedDSYMs(dsymsPath: string): { zipFiles: string[], uploadPath: string } {
const absPath = validateDSYMsPath(dsymsPath);
const { dSYMDirs, dSYMZipFiles } = scanDSYMsDirectory(absPath);

// Create a unique system temp directory for storing zip files
const uploadPath = mkdtempSync(join(tmpdir(), 'splunk_dSYMs_upload_'));

const results: string[] = [];

// Build a Set of `*.dSYM.zip` filenames without the `.zip` extension for quick lookup
const existingZipBasenames = new Set(dSYMZipFiles.map(f => f.replace(/\.zip$/, '')));

for (const dSYMDir of dSYMDirs) {
results.push(zipDSYMDirectory(absPath, dSYMDir, uploadPath));
}

for (const zipFile of dSYMZipFiles) {
const baseName = zipFile.replace(/\.zip$/, '');
if (!existingZipBasenames.has(baseName)) {
// Only copy *.dSYM.zip files that don't have a corresponding *.dSYM/ directory
const srcPath = join(absPath, zipFile);
const destPath = join(uploadPath, zipFile);
try {
copyFileSync(srcPath, destPath);
} catch (err) {
throwAsUserFriendlyErrnoException(err, {
ENOENT: `Failed to copy ${srcPath} to ${destPath}. Please ensure the file exists and is not in use.`,
EACCES: `Permission denied while copying ${srcPath}. Please check your access rights.`,
});
}
results.push(destPath);
}
}

return { zipFiles: results, uploadPath };
}