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

Revert "The return of the "Generate precompilation as part of build process #28346

Merged
merged 1 commit into from
Jul 29, 2018

Conversation

KristofferC
Copy link
Sponsor Member

While #28319 passes tests and provide a good REPL experience with building locally it has one problem that I failed to realize before merging.
The generator script implicitly assumes that stdin is a TTY in order for the record of the startup process to be the same as for a user. This works well if you build locally but the for the buildbots this is not the case (stdin is redirected to a file or something). This means that the recorded precompilation statements are non-accurate and this harms startup time and REPL startup time compared to the hardcoded precompilation statements.

It is obvious that something is off by looking at the number of precompilation statements that gets generated locally (~730) vs on the buildbots (~650).

So I think it is best to revert this and get a proper version going that creates its own TTY and have no assumptions on the building environment.

I apologize for the frequent merging / revert cycle lately :(

@JeffBezanson
Copy link
Sponsor Member

Can we fix it without reverting first? This is a lot of churn.

@KristofferC
Copy link
Sponsor Member Author

Yeah, I apologize for it, I should have tested more carefully (gotten a full buildbot artifact from the PR and see that it had good results before merging).

But regarding a lot of churn, it will be 4 commits that look a bit stupid but I'm not sure what the actual harm is? Right now, I just feel bad that my PR that was supposed to help load time is doing the opposite.

Regarding creating a TTY, it is likely that I can adapt the code in https://github.com/Keno/VT100.jl/blob/a13872010a92c9827e8e1d21c0eed8dadda2c895/src/VT100.jl#L557-L590 to work for this in Linux and probably Mac but getting it to work on Windows as well might take some time.

If it is ok I rather just get this revert through and then I'll make sure it is correct and good the next time.

@KristofferC KristofferC merged commit 78cab70 into master Jul 29, 2018
@KristofferC
Copy link
Sponsor Member Author

Again, sorry for the churn and having to revert.

@KristofferC KristofferC deleted the kc/nooo branch July 29, 2018 22:13
@JeffBezanson
Copy link
Sponsor Member

It's ok, I just feel lots of reverts make the commit history messy. It also means the repl in my locally-built version has to be slower again temporarily :-P

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jul 30, 2018

KristofferC added a commit that referenced this pull request Feb 11, 2019
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