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

dub run <package> should search the registry on failure #1082

Closed
wants to merge 1 commit into from

Conversation

wilzbach
Copy link
Member

@wilzbach wilzbach commented Feb 23, 2017

Adds a simple fallback to dub run, s.t. the following pattern simply works:

dub run dlang-tour
dub run dfmt
dub run dscanner
dub run avgtime
...

I am not sure how we can best test this without querying the registry. Also I don't know any package that is compatible with all the compilers that DUB supports.
What would be the best strategy here?

@wilzbach wilzbach mentioned this pull request Feb 23, 2017
@s-ludwig
Copy link
Member

What about making this slightly interactive and requiring the user to confirm the download. This would also be a chance to print the package description and maybe the github address, to make it less likely to make this en entry point for malicious code.

There is http://code.dlang.org/packages/gitcompatibledubpackage which is used in some of the tests already. We'd just have to add an executable configuration (in this case just a source/app.d).

@wilzbach
Copy link
Member Author

What about making this slightly interactive and requiring the user to confirm the download. This would also be a chance to print the package description and maybe the github address, to make it less likely to make this en entry point for malicious code.

Good idea - I gave it a shot. It's not perfect because without more refactoring the info payload will get downloaded twice from the registry.

There is http://code.dlang.org/packages/gitcompatibledubpackage which is used in some of the tests already. We'd just have to add an executable configuration

https://github.com/MartinNowak/gitcompatibledubpackage/pull/1/files

I also added a test for this -> Travis will fail for a bit.

@MartinNowak
Copy link
Member

https://github.com/MartinNowak/gitcompatibledubpackage/pull/1/files

I also added a test for this -> Travis will fail for a bit.

Merged

@s-ludwig
Copy link
Member

s-ludwig commented Apr 6, 2017

Appears that issue877-auto-download-package-if-not-found-on-error.sh is still missing an echo "y" |, or maybe a command line option that does the same. Ideally, it would also detect that stdin is empty (does that work using an EOF check?) and assume "n" in that case (would be backwards compatible and avoids unsolicited downloads).

set -e -o pipefail

$DUB remove --version='*' cpuid > /dev/null 2>&1 || true
$DUB run dfmt
Copy link
Member

Choose a reason for hiding this comment

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

How is that different from the test below?

if (json == Json.emptyObject)
return 2;

writefln("%s wasn't found online, but there's version available online:", package_name);
Copy link
Member

Choose a reason for hiding this comment

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

dfmt wasn't found online, but there's version available online???

writefln("%s wasn't found online, but there's version available online:", package_name);
if ("authors" in json)
writefln("Authors: %-(%s, %)", json["authors"][].map!(v => v.get!string));
if ("license" in json)
Copy link
Member

Choose a reason for hiding this comment

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

How about

if (auto p = "license" in json)
    writefln("Description: %s", p.get!string);

if ("version" in json)
writefln("Version: %s", json["version"].get!string);
if ("description" in json)
writefln("License: %s", json["license"].get!string);
Copy link
Member

Choose a reason for hiding this comment

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

Description and License are mixed up.

default:
}
} else {
nb = true;
Copy link
Member

Choose a reason for hiding this comment

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

Should be the default value, no?

nb = true;
case "no", "n":
nb = false;
default:
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather loop in here, than on the outside. Also allows you to drop Nullable.

logInfo("Retry with ~master...");
dep = Dependency("~master");
json = dub.info(package_name, dep, fetchOpts, supplier);
}
Copy link
Member

Choose a reason for hiding this comment

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

Could this maybe reuse dub.searchPackages?

auto package_name = free_args[0];
auto pack = dub.packageManager.getFirstPackage(package_name);

if (!pack)
Copy link
Member

Choose a reason for hiding this comment

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

Please check some --non-interactive argument as well to prevent hanging scripts.

PetarKirov added a commit to PetarKirov/Digger that referenced this pull request Jun 29, 2017
Dub packages must first be fetched before you can run them, at least
until dlang/dub#1082 is accepted.
CyberShadow pushed a commit to CyberShadow/Digger that referenced this pull request Jun 29, 2017
Dub packages must first be fetched before you can run them, at least
until dlang/dub#1082 is accepted.
@dlang-bot
Copy link
Collaborator

Thanks for your pull request, @wilzbach!

@wilzbach wilzbach force-pushed the fix-877 branch 3 times, most recently from 473f67a to f6cf601 Compare September 14, 2017 22:59
@wilzbach
Copy link
Member Author

Okay I finally found time to rebase and rework this. Thanks to @MartinNowak's feedback and the time passed since I submitted this, I managed to cut the size of this PR in half :)

This only needs a new release of gitcompatibledubpackage as dlang-community/gitcompatibledubpackage#1 isn't part of the dummy package's 1.0.2 release (@MartinNowak: how about moving this package to dlang-community?)


bool answer = input("Do you want to fetch %s?".format(package_name));
auto dep = Dependency(p.version_);
dub.fetch(package_name, dep, dub.defaultPlacementLocation, fetchOpts);
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: since the package doesn't exist locally, FetchOptions.forceBranchUpgrade shouldn't have an effect and FetchOptions.none would be sufficient here.

@wilzbach
Copy link
Member Author

I accidentally closed this PR because I wanted to trigger a notification to Semaphore while I pushed and GitHub doesn't seem to like this (even restoring the old commit sha still doesn't allow me to reopen).

Anyway, I opened a new PR -> #1428
Sorry for the noise.

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.

4 participants