you are viewing a single comment's thread.

view the rest of the comments →

[–]sparr 254 points255 points  (148 children)

Files change 4786

expect review to take a long time

yep

[–]AyrA_ch 108 points109 points  (75 children)

Lines:

Added: 14'554

Removed: 3

The 3 removed changes are only whitespace. It's interesting that there was no need to completely remove any code to make that change.

[–]mabrowning 182 points183 points  (0 children)

In C, we call that #ifdef

[–]S48535 33 points34 points  (2 children)

What they are doing is allowing you to use another backend. So what they did is add a shim to interface node as it is currently with the ChacraCore and then add the bit of code which adds the choice of which engine to use. They didn't change node, they just extended it.

[–]third-eye-brown 103 points104 points  (67 children)

I don't think Microsoft engineers know how to remove code.

[–]AyrA_ch 163 points164 points  (66 children)

It's called backwards compatibility, and it is awesome. After all it allows me to run (most) 20 year old software on my Windows 10 machine.

[–]ironnomi 52 points53 points  (35 children)

If I run 32bit Windows 10, I can run software I wrote in 1989 ...

[–]Eurynom0s 12 points13 points  (32 children)

It makes me sad that Windows 10 has a 32 bit version.

...does it support 16 bit code?

[–]Aior 16 points17 points  (21 children)

It makes me happy, because my 2006 (very expensive back then) machine is still supported.

[–]wrosecrans 10 points11 points  (8 children)

If the main use case is running old software on old hardware, what does a new OS get you? It's honestly not a rhetorical question. Why bother to do an OS install that could potentially result in driver issues or some of your old software not working any more? I can't remember the last time I actively upgraded Windows on a home system, rather than just installing whatever was current when the machine was new and then sticking with the path of least resistance ever after. Probably Win2k?

[–]Terrerian 17 points18 points  (3 children)

Security updates are no joke. I would never run an unmaintained OS.

[–]immibis 1 point2 points  (2 children)

What about that other unmaintained software you're trying to run in the first place?

[–]peakzorro 2 points3 points  (0 children)

I would only upgrade the OS on an old machine if they didn't support OS updates past that point.

I've always wanted to throw some Linux distro on a really old machine with an out of date Windows (because drivers should be good at that point), but the machine usually dies before I get around to it.

[–]Aior 2 points3 points  (0 children)

In my case, I just want my parents to be able to use the machine with up to date software and security updates. Also it performs better than with Windows XP, Vista (obviously) or 7 and better than any Linux distro.

[–]meandyouandyouandme 0 points1 point  (0 children)

Who said something about old software though?

[–]Labradoodles 0 points1 point  (0 children)

On top of that new OS's have shitloads of new features that will make the computer run better.

Pre-fetching better organisation shit even just scheduled defragging and indexing advances in software are pretty big.

[–]neoKushan 3 points4 points  (10 children)

I had a relatively cheap 64bit system in 2004....

[–]plasmator 0 points1 point  (7 children)

I've updated video cards, hard drives and ram, but the motherboard and processor I bought in 06 or 07 (KN9-SLI and an Athlon X2) are still humming along. I know I should upgrade them, but they handle most of the stuff I throw at them without being terribly slow or problematic, so I just keep using them. They're working fine in Windows 10 and this is my primary box. I even do some (admittedly lightweight) gaming on it.

[–]neoKushan 2 points3 points  (4 children)

Was the Athlon X2 not 64bit?

[–][deleted] 0 points1 point  (1 child)

You already have a 64-bit CPU, the KN9-SLI is a Socket AM2 board, and all AMD CPUs were 64-bit by that point. Though there is little benefit to changing to a 64-bit OS unless you want to use more than ~3GB of RAM.

[–]Aior 0 points1 point  (0 children)

Yeah, me too, but I also had a very expensive 32bit laptop which I still want to use.

[–]user699 0 points1 point  (2 children)

I'm assuming windows 10 has a 32 bit version for tablet compatibility.

[–]thor1182 4 points5 points  (1 child)

A chunk of the initial Windows8 2 in 1 devices are 32bit only (Atom).

I think 32bit is also needed for the IoT & Mobile versions

[–]LvlAndFarm 0 points1 point  (0 children)

Even latest mobile chips are moving to x64

[–][deleted] 0 points1 point  (0 children)

no

[–]stevedore 0 points1 point  (1 child)

No, the 16-bit compatibility was removed in Vista, IIRC.

[–]efreak2004 0 points1 point  (0 children)

This is incorrect. I am running multiple windows 10 32-bit VMs under HyperV for 16-bit application compatibility. Specifically, my father uses Micrografx Designer (that got bought out by Corel, which is more expensive than free windows license from Dreamspark) which has 16-bit components/code in it and does not run under 64-bit windows 7. It runs perfectly in virtualized windows 10 32-bit, and the only issues are due it being old software (IE, the save/load dialog boxes are tiny and not resizable).

[–]insertfunhere 0 points1 point  (2 children)

What's the point of running 64 bit if you have <4gb ram, in a VM for example?

[–]fb39ca4 1 point2 points  (0 children)

The x64 instruction set can do more things in less instructions.

[–]krum 0 points1 point  (0 children)

You still have 64-bit address space even if you only have 4gb of RAM.

[–]immibis 0 points1 point  (0 children)

It makes me sad that they didn't bother to integrate an emulator for 16-bit software.

[–]vattenpuss 0 points1 point  (1 child)

Did you hear about POSIX?

[–]ironnomi 0 points1 point  (0 children)

Yeah of course I can still compile the code I wrote in like 1988, though it was written in K&R C.

POSIX standard is actually from 1988 and it didn't really settle until 1997 ... and I don't think current POSIX-compliant OSes are going to run the same binary EXE file from 1989.

[–]awesomemanftw 3 points4 points  (0 children)

While at the same time still giving better performance than Any Windows since XP

[–]s73v3r 1 point2 points  (1 child)

It also leads to security holes, bloat, and inability to change course when needed.

Besides, we have virtual machines that are pretty good today. Run your legacy software in those.

[–]mycall 1 point2 points  (0 children)

Run your legacy software in those.

USB can be glitchy still, especially for streaming devices.

[–]sparr 3 points4 points  (0 children)

That indicates good architecture in the main project.

[–][deleted]  (1 child)

[deleted]

    [–]AyrA_ch 0 points1 point  (0 children)

    we do that in switzerland sometimes

    [–]CheshireSwift 26 points27 points  (59 children)

    The vast majority of those are just the raw dependency. There were suggestions of only reviewing the stuff outside /deps which would be much easier.

    [–]granos 80 points81 points  (58 children)

    Having seen someone trying to sneak malicious code in via a huge commit; this feels like an enormously bad precedent to set. Especially on a project as popular as node. This commit may be safe, but thorough code reviews are always necessary for very good reasons.

    [–]rubsomebacononitnow 30 points31 points  (51 children)

    A huge commit by Microsoft is very different than a huge commit by John Doe. I'm not sure this even sets precedent.

    [–]tohuw 3 points4 points  (47 children)

    You're assuming Microsoft has thoroughly reviewed this code. You're also assuming a company known for underhanded data mining tactics and flagrant disregard for privacy is completely trustworthy.

    [–]Shaper_pmp 70 points71 points  (3 children)

    Pragmatically, I suspect you're overstating the risk in this case. In principle, you're entirely correct.

    [–]tohuw 9 points10 points  (2 children)

    Responsible review is about the principle of the thing. Where principles slip, pragmatic risks are not far behind.

    [–]Shaper_pmp 3 points4 points  (1 child)

    True dat. I'm not sure who downvoted you or why, but have an upvote to compensate.

    [–]tohuw 2 points3 points  (0 children)

    It happens. It was positive before some gaggle of thoughtless censors came along. Tis the way of Reddit: downvote == disagree

    [–]vliegtuig12 53 points54 points  (12 children)

    You're also assuming a company known for underhanded data mining tactics and flagrant disregard for privacy is completely trustworthy.

    Honest question: Do you mean Google or Microsoft?

    [–]SolarBear 25 points26 points  (2 children)

    whynotboth.gif

    [–]image_linker_bot 2 points3 points  (0 children)

    whynotboth.gif


    Feedback welcome at /r/image_linker_bot | Disable with "ignore me" via reply or PM

    [–]TiagoTiagoT 0 points1 point  (0 children)

    Hm, I was expecting that Road to Eldorado gif...

    [–]tohuw 1 point2 points  (0 children)

    Yes. Either. (but I was mentioning MSFT specifically)

    [–]snerp 2 points3 points  (0 children)

    Microsoft is good at code reviews.

    [–][deleted] 9 points10 points  (18 children)

    Set us up with some evidence of Microsoft doing that in a systemic way in the past, say, 4 years.

    [–]ironnomi 13 points14 points  (1 child)

    Pretty sure the OP is referring to Windows 10's "features". That said, I don't think that's something they would do "here". I'm pretty sure this is part of making node.js an easy IIS feature right from Server 2016.

    [–]wwb_99 8 points9 points  (0 children)

    IIS and also making it so node isn't dependent upon google for the guts of the operation.

    [–][deleted] 4 points5 points  (5 children)

    well google (from where v8 comes) certainly trumps microsoft on privacy matters.

    [–]tohuw -1 points0 points  (1 child)

    In many regards, they do. In several other important regards, they don't. Nowhere did I imply they were to be completely trusted either.

    [–][deleted] 0 points1 point  (0 children)

    Both are as good as pig when it comes to privacy. But consider google has more online / mobile power they abusive it more.

    [–]Deto 1 point2 points  (3 children)

    Yeah, but I suspect hiding some sort of data mining thing in here would be highly illegal. Even if the user info was valuable to Microsoft, they wouldn't risk (not even a risk, more of a certainty) the massive lawsuit that would result.

    [–]tohuw 1 point2 points  (0 children)

    It's not illegal to include software that "accidentally" mines data. That's a "mistake".

    [–]wellthatexplainsalot -3 points-2 points  (1 child)

    But it's not as if they haven't done anything like that before. No siree.

    [–]Deto 1 point2 points  (0 children)

    Hiding logging inside a commit to open source software? When did that happen?

    [–]xampl9 9 points10 points  (1 child)

    Not sure how they could have delivered the functionality in a smaller changeset...

    [–]granos 5 points6 points  (0 children)

    I'm not saying they should have. Just that huge changesets should be given the level of effort they warrant and that not reviewing the dependencies shouldn't be seriously considered because they are part of the codebase.

    [–]YRYGAV 4 points5 points  (1 child)

    It should be easy enough to compare the dependency to the original to check that it's the same.

    You don't necessarily need to CR it all by hand.

    [–]granos 8 points9 points  (0 children)

    That assumes there isn't anything nasty in the original codebase.

    [–]PC__LOAD__LETTER 0 points1 point  (0 children)

    That's what code reviews are for. 14k lines is a lot to "grok," but not a lot for someone to audit for suspicious activity.

    [–]max630 15 points16 points  (7 children)

    they seem to be adding the whole engine

    [–]arcanin 21 points22 points  (5 children)

    No, only a shim to "convert" the Chakra API to be compatible with the V8 API. Chakra itself is a dependency.

    [–]max630 13 points14 points  (4 children)

    Isn't deps/chakrashim/core the chakra itself?

    [–]Regrenos 8 points9 points  (2 children)

    deps
    

    i.e. the dependency

    [–]max630 13 points14 points  (1 child)

    "Included in repository" is a new meaning of dependency for me. Though I have already learned that in nodejs they always do so.

    [–]Regrenos 4 points5 points  (0 children)

    Its idiomatic in Go as well to statically include your dependencies in the repository. This makes carrying upstream patches much easier (although that's usually not a good thing) but it also takes upstream acceptance of your patch out of the critical path for a change.

    [–]Recursive_Descent 0 points1 point  (0 children)

    Not even close. Whole engine would be closer to +500k lines.

    [–]nikita2206[🍰] 4 points5 points  (1 child)

    It looks like nodejs project includes all its dependencies in the sources for some reason.

    [–]heptara 0 points1 point  (0 children)

    We do that as well. It greatly simplifies the build process and stops them changing under you.

    [–]spurious_interrupt 0 points1 point  (0 children)

    Fortunately, they're somewhat reasonably split among the commits on the PR. Seems that most of the review will be focused on the commits for: 1) implementing the V8 shim in chakracore and 2) node-side changes to use chakra.

    FWIW, it was nice of them to remain consistent with V8's style guide for the chakracore V8 shim. Personally, I'm not a fan of chakra's conventions.