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

feat(.net): embed package icon when configured #3676

Merged
merged 6 commits into from
Aug 4, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion packages/@scope/jsii-calc-base/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,8 @@
},
"dotnet": {
"namespace": "Amazon.JSII.Tests.CalculatorNamespace.BaseNamespace",
"packageId": "Amazon.JSII.Tests.CalculatorPackageId.BasePackageId"
"packageId": "Amazon.JSII.Tests.CalculatorPackageId.BasePackageId",
"iconUrl": "https://raw.githubusercontent.com/aws/aws-cdk/main/logo/default-256-dark.png"
},
"python": {
"distName": "scope.jsii-calc-base",
Expand Down
3 changes: 2 additions & 1 deletion packages/@scope/jsii-calc-base/test/assembly.jsii
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
"schema": "jsii/0.10.0",
"targets": {
"dotnet": {
"iconUrl": "https://raw.githubusercontent.com/aws/aws-cdk/main/logo/default-256-dark.png",
"namespace": "Amazon.JSII.Tests.CalculatorNamespace.BaseNamespace",
"packageId": "Amazon.JSII.Tests.CalculatorPackageId.BasePackageId"
},
Expand Down Expand Up @@ -206,5 +207,5 @@
}
},
"version": "0.0.0",
"fingerprint": "DVCANvLLzJEu5VNOJVmldbT5wTXhmHlZzk3E6muTR5I="
"fingerprint": "HiHbzAfeO1CAYQhgLFruW7OWwx9db1OpnC4Pl3Vlqak="
}
3 changes: 2 additions & 1 deletion packages/@scope/jsii-calc-lib/test/assembly.jsii
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
"@scope/jsii-calc-base": {
"targets": {
"dotnet": {
"iconUrl": "https://raw.githubusercontent.com/aws/aws-cdk/main/logo/default-256-dark.png",
"namespace": "Amazon.JSII.Tests.CalculatorNamespace.BaseNamespace",
"packageId": "Amazon.JSII.Tests.CalculatorPackageId.BasePackageId"
},
Expand Down Expand Up @@ -953,5 +954,5 @@
}
},
"version": "0.0.0",
"fingerprint": "wtswDMhtuMcC4+ami5KddQIznqdpVjt8qc7Ba2QnnHk="
"fingerprint": "kYLYyNyPod3JTyJWmgPJL1Z85k0HEEhQeMIs4zH4bEQ="
}
3 changes: 2 additions & 1 deletion packages/jsii-calc/test/assembly.jsii
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
"@scope/jsii-calc-base": {
"targets": {
"dotnet": {
"iconUrl": "https://raw.githubusercontent.com/aws/aws-cdk/main/logo/default-256-dark.png",
"namespace": "Amazon.JSII.Tests.CalculatorNamespace.BaseNamespace",
"packageId": "Amazon.JSII.Tests.CalculatorPackageId.BasePackageId"
},
Expand Down Expand Up @@ -17652,5 +17653,5 @@
}
},
"version": "3.20.120",
"fingerprint": "/9fE/+p2My22F013iCie8G2N8WE1ZwPqZKkeGKYCbyI="
"fingerprint": "P+E1ta4n5zaxREzT7CqW574NyC10CNmKv5NAwZ5pRt8="
}
131 changes: 130 additions & 1 deletion packages/jsii-pacmak/lib/targets/dotnet/dotnetgenerator.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
import * as spec from '@jsii/spec';
import * as clone from 'clone';
import * as fs from 'fs-extra';
import * as http from 'http';
import * as https from 'https';
import * as reflect from 'jsii-reflect';
import { Rosetta } from 'jsii-rosetta';
import * as path from 'path';

import { Generator, Legalese } from '../../generator';
import { debug } from '../../logging';
import { MethodDefinition, PropertyDefinition } from '../_utils';
import { DotNetDocGenerator } from './dotnetdocgenerator';
import { DotNetRuntimeGenerator } from './dotnetruntimegenerator';
Expand Down Expand Up @@ -89,7 +92,7 @@ export class DotNetGenerator extends Generator {
this.code,
);
filegen.generateAssemblyInfoFile();
filegen.generateProjectFile(this.typeresolver.namespaceDependencies);

// Calling super.save() dumps the tarball in the format name@version.jsii.tgz.
// This is not in sync with the Old .NET generator where the name is scope-name-version.tgz.
// Hence we are saving the files ourselves here:
Expand All @@ -103,6 +106,27 @@ export class DotNetGenerator extends Generator {
await fs.mkdirp(path.join(outdir, packageId));
await fs.copyFile(tarball, path.join(outdir, packageId, tarballFileName));

// Attempt to download the package icon from the configured URL so we can use the non-deprecated PackageIcon
// attribute. If this fails or is opted out (via $JSII_PACMAK_DOTNET_NO_DOWNLOAD_ICON being set), then only the
// deprecated PackageIconUrl will be emitted.
const iconFile =
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way for the user to pass this file without downloading it (eg if they already have the icon locally)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not currently... That'd require they bundle the icon in their npm package, which I'm not sure is appropriate...

this.assembly.targets?.dotnet?.iconUrl != null &&
!process.env.JSII_PACMAK_DOTNET_NO_DOWNLOAD_ICON
? await tryDownloadResource(
this.assembly.targets.dotnet.iconUrl,
path.join(outdir, packageId),
).catch((err: any) => {
debug(
`[dotnet] Unable to download package icon, will only use deprecated PackageIconUrl attribute: ${err.cause}`,
);
return Promise.resolve(undefined);
comcalvi marked this conversation as resolved.
Show resolved Hide resolved
})
: undefined;
filegen.generateProjectFile(
this.typeresolver.namespaceDependencies,
iconFile,
);

// Create an anchor file for the current model
this.generateDependencyAnchorFile();

Expand Down Expand Up @@ -1191,3 +1215,108 @@ export class DotNetGenerator extends Generator {
);
}
}

async function tryDownloadResource(
urlText: string,
into: string,
): Promise<string | undefined> {
const url = new URL(urlText);

let request: typeof http.get | typeof https.get;
switch (url.protocol) {
case 'http:':
request = http.get;
break;
case 'https:':
request = https.get;
break;
default:
// Unhandled protocol... ignoring
debug(
`Unsupported URL protocol for resource download: ${url.protocol} (full URL: ${urlText})`,
comcalvi marked this conversation as resolved.
Show resolved Hide resolved
);
return undefined;
}

return new Promise((ok, ko) =>
request(url, (res) => {
switch (res.statusCode) {
case 200:
let fileName = path.basename(url.pathname);
// Ensure there is a content-appropriate extension on the result...
switch (res.headers['content-type']) {
case 'image/gif':
if (!fileName.endsWith('.gif')) {
fileName = `${fileName}.gif`;
}
break;
case 'image/jpeg':
if (!fileName.endsWith('.jpg')) {
fileName = `${fileName}.jpg`;
}
break;
case 'image/png':
if (!fileName.endsWith('.png')) {
fileName = `${fileName}.png`;
}
break;
default:
// Nothing to do...
Copy link
Contributor

Choose a reason for hiding this comment

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

if it's not one of these, how can it be a valid icon? Shouldn't this be an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The spec unfortunately does not specify what is a valid MIME type for the icon, and there are more image formats that I can't be bothered with (ico, heic, etc..)

}
const filePath = path.join('resources', fileName);
try {
fs.mkdirpSync(path.join(into, 'resources'));
} catch (err) {
return ko(err);
}
try {
const fd = fs.openSync(
path.join(into, filePath),
fs.constants.O_CREAT |
fs.constants.O_TRUNC |
fs.constants.O_WRONLY,
);
res
.once('error', (cause) => {
try {
fs.closeSync(fd);
} catch {
// IGNORE
}
ko(cause);
})
.on('data', (chunk) => {
const buff = Buffer.from(chunk);
let offset = 0;
while (offset < buff.length) {
try {
offset += fs.writeSync(fd, buff, offset);
} catch (err) {
return ko(err);
}
}
})
.once('close', () => {
try {
fs.closeSync(fd);
ok(filePath);
} catch (err) {
ko(err);
}
});
} catch (err) {
return ko(err);
}
break;
default:
ko(
new Error(
`GET ${urlText} -- HTTP ${res.statusCode ?? 0} (${
res.statusMessage ?? 'Unknown Error'
})`,
),
);
}
}).once('error', ko),
);
}
21 changes: 20 additions & 1 deletion packages/jsii-pacmak/lib/targets/dotnet/filegenerator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,10 @@ export class FileGenerator {
}

// Generates the .csproj file
public generateProjectFile(dependencies: Map<string, DotNetDependency>) {
public generateProjectFile(
dependencies: Map<string, DotNetDependency>,
iconFile?: string,
) {
const assembly = this.assm;
const packageId: string = assembly.targets!.dotnet!.packageId;

Expand All @@ -62,6 +65,22 @@ export class FileGenerator {

propertyGroup.comment('Package Identification');
propertyGroup.ele('Description', this.getDescription());
if (iconFile != null) {
propertyGroup.ele('PackageIcon', iconFile.split(/[/\\]+/).join('\\'));
// We also need to actually include the icon in the package
const noneNode = rootNode.ele('ItemGroup').ele('None');
noneNode.att('Include', iconFile.split(/[/\\]+/).join('\\'));
noneNode.att('Pack', 'true');
noneNode.att(
'PackagePath',
`\\${path
.dirname(iconFile)
.split(/[/\\]+/)
.join('\\')}`,
);
}
// We continue to include the PackageIconUrl even if we put PackageIcon for backwards compatibility, as suggested
// by https://docs.microsoft.com/en-us/nuget/reference/msbuild-targets#packageicon
if (dotnetInfo!.iconUrl != null) {
propertyGroup.ele('PackageIconUrl', dotnetInfo!.iconUrl);
}
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions packages/jsii-pacmak/test/generated-code/harness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { shell } from '../../lib/util';
const FILE = Symbol('file');
const MISSING = Symbol('missing');
const TARBALL = Symbol('tarball');
const BINARY = Symbol('binary');
export const TREE = Symbol('tree');

// Custom serializers so we can see the source without escape sequences
Expand All @@ -28,6 +29,10 @@ expect.addSnapshotSerializer({
val[TARBALL].endsWith('.tgz') ? 'is' : 'embeds'
} a tarball`,
});
expect.addSnapshotSerializer({
test: (val) => val?.[BINARY] != null,
serialize: (val) => `${val[BINARY]} is a binary file`,
});
expect.addSnapshotSerializer({
test: (val) => val?.[TREE] != null,
serialize: (val) => {
Expand Down Expand Up @@ -101,6 +106,9 @@ export function checkTree(
if (file.endsWith('.tgz')) {
// Special-cased to avoid binary differences being annoying
expect({ [TARBALL]: relativeFile }).toMatchSnapshot(snapshotName);
} else if (file.endsWith('.png')) {
// Special-cased to avoid binary differences being annoying
expect({ [BINARY]: relativeFile }).toMatchSnapshot(snapshotName);
} else {
expect({
[FILE]: fs.readFileSync(file, { encoding: 'utf-8' }),
Expand Down