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

Path fix for node-js on Ubuntu (or Debian in general?) #3777

Merged
merged 1 commit into from
Jun 11, 2018

Conversation

benjaoming
Copy link
Contributor

Summary

This was previously attempted in #1688 but I found a Node 6 upgrade fixed the issue back then.

However, now on Ubuntu 18.04, I find the issue occurring with the current official Debian pkg for Node.

@rtibbles does this work on your system as well?

If this get's merged, I'm fine that it get's quickly reverted if fellow devs report issues with it.

Reviewer guidance

@rtibbles we've been here before...

References

#1688


Contributor Checklist

  • Contributor has fully tested the PR manually (on MY SYSTEM)
  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If there are any front-end changes, before/after screenshots are included
  • If this is an important user-facing change, PR or related issue has a 'changelog' label

Reviewer Checklist

  • Automated test coverage is satisfactory
  • Reviewer has fully tested the PR manually
  • PR has been tested for accessibility regressions
  • External dependencies files were updated (yarn and pip)
  • Documentation is updated
  • Link to diff of internal dependency change is included
  • CHANGELOG.rst is updated for high-level changes
  • Contributor is in AUTHORS.rst

@benjaoming benjaoming added TODO: needs review Waiting for review DEV: dev-ops Continuous integration & deployment TAG: dev experience Build performance, linting, debugging... labels Jun 8, 2018
@benjaoming benjaoming added this to the 0.11.0 milestone Jun 8, 2018
@benjaoming benjaoming requested a review from rtibbles June 8, 2018 16:39
@@ -9,7 +9,20 @@ module.exports = function() {
var webpack_json_tempfile = temp.openSync({ suffix: '.json' }).path;

// Run the script below to extract the relevant information about the plugin configuration from the Python code.
execSync(`python ${webpack_json} --output_file ${webpack_json_tempfile}`);
// execSync(`python ${webpack_json} --output_file ${webpack_json_tempfile}`);

This comment was marked as spam.

This comment was marked as spam.

@indirectlylit
Copy link
Contributor

indirectlylit commented Jun 9, 2018

My guess is this is caused by yarnpkg/yarn#5874

One person writes:

Basically the Python virtualenv doesn't work anymore -- Python scripts we've written that are executed with yarn can no longer import the right packages since its (presumably) using a different Python installation than it used to.

FWIW this seems to affect our Linux (Ubuntu) systems but not our MacOS systems; both use virtualenv.

@codecov
Copy link

codecov bot commented Jun 9, 2018

Codecov Report

Merging #3777 into develop will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3777      +/-   ##
===========================================
- Coverage    50.24%   50.23%   -0.01%     
===========================================
  Files          593      593              
  Lines        19829    19831       +2     
  Branches      2476     2477       +1     
===========================================
  Hits          9963     9963              
- Misses        9319     9321       +2     
  Partials       547      547
Impacted Files Coverage Δ
frontend_build/src/read_webpack_json.js 40% <0%> (-6.16%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9162c30...fcb223b. Read the comment docs.

@benjaoming
Copy link
Contributor Author

My guess is this is caused by yarnpkg/yarn#5874

That sounds like the right upstream cause! Thanks for finding it @indirectlylit ! I'll remain subscribed to the issue in order to revert these changes once fixed!

@indirectlylit
Copy link
Contributor

cool. mind updating the "for reasons unknown" comment?

@benjaoming
Copy link
Contributor Author

Sure, done!

@indirectlylit indirectlylit merged commit 4385042 into learningequality:develop Jun 11, 2018
@indirectlylit indirectlylit removed TODO: needs review Waiting for review labels Oct 12, 2018
@benjaoming benjaoming deleted the bug/node-path branch October 12, 2018 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: dev-ops Continuous integration & deployment TAG: dev experience Build performance, linting, debugging...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants