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.

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

Jump to bottom

workspace command: fix for missing incremental support #21714

Merged
merged 2 commits into from
Jun 1, 2017
Merged

workspace command: fix for missing incremental support #21714

merged 2 commits into from
Jun 1, 2017

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented May 5, 2017

I can't really spend the time to analyze the consequences of calling workspace(), but this at least makes it not fail too quickly.

@vtjnash vtjnash added backport pending 0.6 bugfix This change fixes an existing bug labels May 5, 2017
@tkelman tkelman added the needs tests Unit tests are required for this change label May 5, 2017
@tkelman
Copy link
Contributor

tkelman commented May 18, 2017

would this fix #21920 ?

@vtjnash
Copy link
Member Author

vtjnash commented May 18, 2017

not sure. it fixed tests for some package, but I seem to have forgotten to list the bug number

@vchuravy vchuravy mentioned this pull request May 28, 2017
Closed
@ararslan
Copy link
Member

I can confirm that this fixes #22101. We should definitely have a test for this though.

@tkelman tkelman added this to the 0.6.0 milestone May 28, 2017
@ararslan ararslan removed the needs tests Unit tests are required for this change label May 30, 2017
@ararslan
Copy link
Member

Hm, Windows apparently doesn't like my test.

vtjnash
vtjnash commented May 31, 2017
View reviewed changes
test/workspace.jl Outdated
""")
write(joinpath(dir, "testdriver.jl"), """
insert!(LOAD_PATH, 1, "$dir")
insert!(Base.LOAD_CACHE_PATH, 1, "$dir")
Copy link
Member Author

Choose a reason for hiding this comment

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

need to insert repr(dir) here rather than "$dir"

vtjnash
vtjnash commented May 31, 2017
View reviewed changes
test/workspace.jl Outdated
end
exit(isdefined(Main, :f22101) ? 0 : 1)
""")
@test success(`$(Base.julia_cmd()) --startup-file=no --precompile=yes $(joinpath(dir, "testdriver.jl"))`)
Copy link
Member Author

Choose a reason for hiding this comment

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

might want to pass DevNull, STDOUT, and STDERR

Copy link
Member

Choose a reason for hiding this comment

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

Pass them to what?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess success doesn't take them as arguments, so to pipeline?

Copy link
Member

Choose a reason for hiding this comment

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

Why do DevNull, STDOUT, and STDERR need to be specified? I'm not super familiar with how the piping stuff works.

Copy link
Member Author

Choose a reason for hiding this comment

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

The default is to swallow all stdio, which is why the windows failure didn't print anything

Copy link
Member

Choose a reason for hiding this comment

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

Ah, so passing it helps with debugging the test

@ararslan ararslan force-pushed the jn/fix-for-workspace branch 2 times, most recently from fbd56c3 to f0c058a Compare May 31, 2017 18:33
tkelman
tkelman reviewed Jun 1, 2017
View reviewed changes
test/workspace.jl Outdated
exit(isdefined(Main, :f22101) ? 0 : 1)
""")
# Ensure that STDIO doesn't get swallowed (helps with debugging)
cmd = `$(Base.julia_cmd()) --startup-file=no --precompile=yes $(joinpath(dir, "testdriver.jl"))`
Copy link
Contributor

Choose a reason for hiding this comment

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

there is no --precompile flag, there is a --precompiled

Copy link
Member

Choose a reason for hiding this comment

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

Right, thanks. I probably should have --compilecache=yes to ensure that the module in the tests gets precompiled, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess in case julia_cmd tries to set that flag to no, maybe?

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't hurt to add it anyway, I suppose.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I think --precompiled=yes may be unnecessary, since it's the precompiled module we care about. (Unless I'm misunderstanding the flag.)

Copy link
Contributor

Choose a reason for hiding this comment

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

sysimg might be irrelevant for this test, not positive though

Copy link
Member

Choose a reason for hiding this comment

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

sysimg?

Copy link
Contributor

Choose a reason for hiding this comment

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

--precompiled is about whether or not the precompiled system image is used - you almost always want that these days, except maybe on win32 or freebsd if you want better backtraces in exchange for slower startup

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, thanks

@ararslan
Copy link
Member

ararslan commented Jun 1, 2017

Okay to merge once CI passes?

@ararslan ararslan merged commit 5c90d2e into master Jun 1, 2017
@ararslan ararslan deleted the jn/fix-for-workspace branch June 1, 2017 21:39
tkelman pushed a commit that referenced this pull request Jun 3, 2017
@vtjnash @tkelman
workspace command: fix for missing incremental support (#21714)
f254613 
* workspace command: fix for missing incremental support

* Add a test for #22101

(cherry picked from commit 5c90d2e)
@tkelman tkelman mentioned this pull request Jun 6, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ararslan ararslan ararslan left review comments

@tkelman tkelman tkelman left review comments

Assignees
No one assigned
Labels
bugfix This change fixes an existing bug
Projects
None yet
Milestone
0.6.0
Development

Successfully merging this pull request may close these issues.
3 participants
@vtjnash @tkelman @ararslan