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

Specify deno_dir location with env var DENO_DIR #970

Merged
merged 5 commits into from
Oct 15, 2018

Conversation

amoseui
Copy link
Contributor

@amoseui amoseui commented Oct 12, 2018

Fixes #552

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

Thanks / do you think we could get a test as well? It might necessitate writing tools/deno_dir_test.py to properly pass the env

@amoseui
Copy link
Contributor Author

amoseui commented Oct 13, 2018

Sure, please take a look.
But, I am not sure this is what you expected for.



def main(argv):
deno_dir_test(argv[1])
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO in this file you want to:

pop the current env (and store it)
rmtree(deno_dir)
run deno with no env flag
assert deno_dir doesn't exist
set env flag (to .deno_test)
run deno
assert deno_dir exists etc
rmtree(deno_dir)
reset back env - even in case of failure

Maybe this is more than @ry wanted?

@amoseui
Copy link
Contributor Author

amoseui commented Oct 15, 2018

@ry @hayd Could you take a look again?

I also change appveyor config because of test errors.

It seems that project dir and deno_dir are on c:\ because it's cloned to c:\ by clone_folder variable in .appveyor.yml. On the other hand, build directory is pointed to C:\ by $(APPVEYOR_BUILD_FOLDER) so that test targets are placed on separated partitions.

Is this intended?

I found some issues(#807 and #861) related to below errors.

https://ci.appveyor.com/project/deno/deno/builds/19513800

c:\deno\out\release\deno.exe C:\deno\tests\001_hello.js --reload
c:\deno\out\release\deno.exe C:\deno\tests\002_hello.ts --reload
c:\deno\out\release\deno.exe C:\deno\tests\003_relative_import.ts --reload
c:\deno\out\release\deno.exe C:\deno\tests\004_set_timeout.ts --reload
c:\deno\out\release\deno.exe C:\deno\tests\005_more_imports.ts --reload
c:\deno\out\release\deno.exe C:\deno\tests\006_url_imports.ts --reload
Expected success but got error. Output:
Downloading http://localhost:4545/tests/subdir/mod2.ts
Downloading http://localhost:4545/tests/subdir/print_hello.ts
error TS5009: Cannot find the common subdirectory path for the input files.

Thanks.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - nice work on the test

@ry ry merged commit 15590a0 into denoland:master Oct 15, 2018
ry added a commit to ry/deno that referenced this pull request Oct 20, 2018
- Performance and stability improvements on all platforms.
- Add cwd() and chdir() denoland#907
- Specify deno_dir location with env var DENO_DIR denoland#970
- Make fetch() header compliant with the current spec denoland#1019
- Upgrade TypeScript to 3.1.3
- Upgrade V8 to 7.1.302.4
@ry ry mentioned this pull request Oct 20, 2018
ry added a commit that referenced this pull request Oct 21, 2018
- Performance and stability improvements on all platforms.
- Add cwd() and chdir() #907
- Specify deno_dir location with env var DENO_DIR #970
- Make fetch() header compliant with the current spec #1019
- Upgrade TypeScript to 3.1.3
- Upgrade V8 to 7.1.302.4
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.

None yet

3 participants