fix noJit builds #114

rajatd · 2016-01-15T00:38:00Z

fix noJit builds

Closes #112

msftclas · 2016-01-15T00:38:04Z

Hi @rajatd, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Rajat Dua). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

rajatd · 2016-01-15T00:38:29Z

@dilijev can you review this?

dilijev · 2016-01-15T01:00:28Z

@dotnet-bot test nojit please

dilijev · 2016-01-15T01:02:13Z

lib/Runtime/Language/JavascriptStackWalker.cpp

@@ -818,6 +825,7 @@ namespace Js
                Assert(this->lastInternalFrameInfo.codeAddress);
                this->lastInternalFrameInfo.frameConsumed = true;
            }
+#endif


To make it more clear can you change this to #endif // ENABLE_NATIVE_CODEGEN and the above #endif to #endif // DBG?

Sure. nested #ifdefs- can get confusing

dilijev · 2016-01-15T01:17:26Z

Jenkins is running slowly. I'm confirming locally that the change merged with master compiles and passes tests.

dilijev · 2016-01-15T01:19:22Z

Looks like the test script is looking for the NoJit binaries in the wrong place for the -disableJit config. That may cause the disablejit checks I triggered to fail. I'll commit an updated script. This change builds, so I think this change will be fine as-is, once it can merge with the new test script.

dilijev · 2016-01-15T01:23:48Z

@dotnet-bot test nojit please

dilijev · 2016-01-15T01:28:47Z

LGTM barring checks and the test script issue I mentioned.

davkean · 2016-01-15T01:32:18Z

Please stop queuing more builds of the same changes - all you are doing adding more builds to the queue, making Jenkins take even longer.

Jenkins is a shared service between many teams, so be mindful of resources.

//cc @mmitche

…nDir. Unblocks ref chakra-core#114.

dilijev · 2016-01-15T01:50:46Z

@davkean @mmitche I understand your frustration. We feel it too. In this case I don't believe we're abusing the resource. This change affects the dailies so we need them to run to verify its correctness. As for the extra requeue of all checks, I apologize. I should have reviewed the change first and waited for the update before adding the dailies.

dilijev · 2016-01-15T01:51:22Z

@rajatd All tests pass on my machine when this change is merged on top of #117.

dilijev · 2016-01-15T01:53:10Z

@davkean @mmitche By the way, I've been wondering if there's an easy way to cancel all queued requests that haven't been issued build machines for a given PR? When a new commit is added to a PR and checks re-queue, do the old ones that haven't run yet get cancelled?

mmitche · 2016-01-15T02:02:41Z

It doesn't automatically cancel, but looking at the source I'm led to believe it may be able to be done.

Sent from my Windows Phone

From: Doug Ilijevmailto:notifications@github.com
Sent: ‎1/‎14/‎2016 5:51 PM
To: Microsoft/ChakraCoremailto:ChakraCore@noreply.github.com
Cc: Matt Mitchellmailto:mmitche@microsoft.com
Subject: Re: [ChakraCore] fix noJit builds (#114)

@davkeanhttps://github.com/davkean @mmitchehttps://github.com/mmitche I understand your frustration. We feel it too. In this case I don't believe we're abusing the resource. This change affects the dailies so we need them to run to verify its correctness. As for the extra requeue of all checks, I apologize. I should have reviewed the change first and waited for the update before adding the dailies.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/114#issuecomment-171843211.

dilijev · 2016-01-15T02:08:40Z

I cancelled the daily checks that were already triggered on this one as I believe they will fail until #117 gets merged in. I'll re-queue the dailies once that's done.

davkean · 2016-01-15T02:20:03Z

@dilijev I was referring to these:

I thought these were duplicated because of your double mention of the trigger phrase for your nojit tests, but I was wrong. Apologies. Looking at them, I can't figure out why both sets were triggered:

@mmitche Maybe this is just Jenkins/Github catching up with the original request, and then queue again up when the PR branch was updated? They are so close to each other in time.

…e correct _BinDir. Unblocks ref #114. Merge pull request #117 from dilijev:nojit-test Fix 'jenkins.testone.cmd ... -disablejit' path to use the correct _BinDir. Unblocks ref #114.

dilijev · 2016-01-15T03:26:08Z

@rajatd You're good to merge this one once tests pass.

dilijev · 2016-01-15T03:26:24Z

@dotnet-bot test dailies please

dilijev · 2016-01-15T07:54:37Z

@digitalinfinity Do we currently support ARM builds for the disablejit configuration? Just want to know whether they were supposed to work.

dilijev · 2016-01-15T08:01:07Z

@rajatd I accidentally triggered all the dailies instead of just the relevant ones. Windows 7 daily_x64_debug is a known failing configuration at the moment. I'll trigger another one and then kill it so at least this PR will be yellow instead of red.

dilijev · 2016-01-15T08:01:25Z

@dotnet-bot test Windows 7 daily_x64_debug please

rajatd · 2016-01-15T19:35:35Z

@dilijev , do we have any historical data pertaining to state of arm builds with no-jit?

dilijev · 2016-01-16T00:39:03Z

@rajatd Not according to Jenkins (as we only have failed daily builds so far). @digitalinfinity was working on this before so that's why I tagged him. I guess we could try the commit at the time Hitesh originally merged in the addition of the NoJIT configurations and see what happens there.

See 6649f32
See #4

dilijev · 2016-01-16T00:43:57Z

I tried from that commit with the current jenkins configs I recently corrected.

jenkins.buildone.cmd arm release "/p:BuildJIT=false"

Produces errors:

Chakra.Runtime.Language.lib(arm_Thunks.obj) : error LNK2001: unresolved external symbol "private: static void * (__cdecl*__cdecl
Js::DynamicProfileInfo::EnsureDynamicProfileInfo(class Js::ScriptFunction *))(class Js::RecyclableObject *,struct Js::CallInfo,..
.)" (?EnsureDynamicProfileInfo@DynamicProfileInfo@Js@@CAP6APAXPAVRecyclableObject@2@UCallInfo@2@ZZPAVScriptFunction@2@@Z) [E:\dev
\Chakra3\core\bin\ChakraCore\ChakraCore.vcxproj]
Chakra.Runtime.Language.lib(arm_Thunks.obj) : error LNK2001: unresolved external symbol "private: static void * (__cdecl*__cdecl
Js::InterpreterStackFrame::EnsureDynamicInterpreterThunk(class Js::ScriptFunction *))(class Js::RecyclableObject *,struct Js::Cal
lInfo,...)" (?EnsureDynamicInterpreterThunk@InterpreterStackFrame@Js@@CAP6APAXPAVRecyclableObject@2@UCallInfo@2@ZZPAVScriptFuncti
on@2@@Z) [E:\dev\Chakra3\core\bin\ChakraCore\ChakraCore.vcxproj]
E:\dev\Chakra3\core\Build\VcBuild\bin\arm_release\ChakraCore.dll : fatal error LNK1120: 2 unresolved externals [E:\dev\Chakra3\co
re\bin\ChakraCore\ChakraCore.vcxproj]

dilijev · 2016-01-16T00:45:59Z

Same error as currently. I think your change is good to check in as-is and we'll follow up with the ARM issue in a separate PR.

dilijev · 2016-01-20T17:45:07Z

Committed as a0f58b0

rajatd · 2016-01-20T18:59:01Z

Thanks @dilijev , I was waiting to see the build status turn green

dilijev · 2016-01-20T22:46:07Z

@rajatd It was green before I marked as merged. That's how I knew it was in.

msftclas added the cla-not-required label Jan 15, 2016

dilijev reviewed Jan 15, 2016
View reviewed changes

Fix noJit builds

43fa1a5

rajatd force-pushed the fixNoJit branch from 953b131 to 43fa1a5 Compare January 15, 2016 01:18

dilijev added a commit to dilijev/ChakraCore that referenced this pull request Jan 15, 2016

Fix path to use the correct _BinDir. Unblocks ref chakra-core#114.

50e20bf

dilijev mentioned this pull request Jan 15, 2016

Fix 'jenkins.testone.cmd ... -disablejit' path to use the correct _BinDir. Unblocks ref #114. #117

Merged

dilijev added a commit to dilijev/ChakraCore that referenced this pull request Jan 15, 2016

Fix 'jenkins.testone.cmd ... -disablejit' path to use the correct _Bi…

d7f19a3

…nDir. Unblocks ref chakra-core#114.

dilijev closed this Jan 20, 2016

dilijev added the Merged label Jan 20, 2016

dilijev mentioned this pull request Jan 20, 2016

Daily builds of DisableJIT (x86, x64) are broken. #112

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix noJit builds #114

fix noJit builds #114

rajatd commented Jan 15, 2016

msftclas commented Jan 15, 2016

rajatd commented Jan 15, 2016

dilijev commented Jan 15, 2016

dilijev Jan 15, 2016

rajatd Jan 15, 2016

dilijev commented Jan 15, 2016

dilijev commented Jan 15, 2016

dilijev commented Jan 15, 2016

dilijev commented Jan 15, 2016

davkean commented Jan 15, 2016

dilijev commented Jan 15, 2016

dilijev commented Jan 15, 2016

dilijev commented Jan 15, 2016

mmitche commented Jan 15, 2016

dilijev commented Jan 15, 2016

davkean commented Jan 15, 2016

dilijev commented Jan 15, 2016

dilijev commented Jan 15, 2016

dilijev commented Jan 15, 2016

dilijev commented Jan 15, 2016

dilijev commented Jan 15, 2016

rajatd commented Jan 15, 2016

dilijev commented Jan 16, 2016

dilijev commented Jan 16, 2016

dilijev commented Jan 16, 2016

dilijev commented Jan 20, 2016

rajatd commented Jan 20, 2016

dilijev commented Jan 20, 2016

fix noJit builds #114

fix noJit builds #114

Conversation

rajatd commented Jan 15, 2016

msftclas commented Jan 15, 2016

rajatd commented Jan 15, 2016

dilijev commented Jan 15, 2016

dilijev Jan 15, 2016

Choose a reason for hiding this comment

rajatd Jan 15, 2016

Choose a reason for hiding this comment

dilijev commented Jan 15, 2016

dilijev commented Jan 15, 2016

dilijev commented Jan 15, 2016

dilijev commented Jan 15, 2016

davkean commented Jan 15, 2016

dilijev commented Jan 15, 2016

dilijev commented Jan 15, 2016

dilijev commented Jan 15, 2016

mmitche commented Jan 15, 2016

dilijev commented Jan 15, 2016

davkean commented Jan 15, 2016

dilijev commented Jan 15, 2016

dilijev commented Jan 15, 2016

dilijev commented Jan 15, 2016

dilijev commented Jan 15, 2016

dilijev commented Jan 15, 2016

rajatd commented Jan 15, 2016

dilijev commented Jan 16, 2016

dilijev commented Jan 16, 2016

dilijev commented Jan 16, 2016

dilijev commented Jan 20, 2016

rajatd commented Jan 20, 2016

dilijev commented Jan 20, 2016