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

Ensure lookupDestinationPath will handle relative path #123

Merged
merged 2 commits into from
Mar 11, 2020

Conversation

SparshithNR
Copy link
Contributor

  1. Let lookupDestinationPath handle the destDir and getDestinationPath
  2. added test case
    Addressing [3.0.1] destDir not respected #122

@rwjblue
Copy link
Member

rwjblue commented Feb 5, 2020

Changes seem good, can you look into failing windows CI?

1) broccoli-funnel
       without filtering options
         can properly handle the output path being a broken symlink:
     Error: ENOENT: no such file or directory, scandir 'C:/Users/appveyor/AppData/Local/Temp/1/broccoli-1992c5HOEsyNqA9w/out-1-funnel/'
      at Object.readdirSync (fs.js:790:3)
      at _walkSync (node_modules\broccoli-test-helper\node_modules\walk-sync\index.js:74:18)
      at Function.entries (node_modules\broccoli-test-helper\node_modules\walk-sync\index.js:50:10)
      at Object.readEntries (node_modules\broccoli-test-helper\dist\util.js:96:48)
      at TreeDiff.diff (node_modules\broccoli-test-helper\dist\tree_diff.js:11:32)
      at builder.build.then (node_modules\broccoli-test-helper\dist\output.js:16:27)

@SparshithNR
Copy link
Contributor Author

@rwjblue It is because of nodejs bug. nodejs/node#30538
#119

@rwjblue
Copy link
Member

rwjblue commented Feb 6, 2020

I see. We should still have a passing CI in this repo before moving forward. Why don't we work around the issue?

@rwjblue
Copy link
Member

rwjblue commented Feb 19, 2020

@SparshithNR - I'd really like to get this fixed, can you take a look at making it at least pass?

@SparshithNR
Copy link
Contributor Author

Sorry, @rwjblue for not looking into it. I will make sure I will put some time to get some workaround if possible and update my progress EOD.

@SparshithNR
Copy link
Contributor Author

@rwjblue Fixed the windows test with a workaround. #124

@rwjblue
Copy link
Member

rwjblue commented Feb 20, 2020

Sorry looks like it is still failing

@rwjblue
Copy link
Member

rwjblue commented Feb 20, 2020

Merged the work around #124, can you rebase?

1. Let lookupDestinationPath handle the destDir and getDestinationPath
2. added test case
@SparshithNR
Copy link
Contributor Author

Rebased and updated the PR.

@rwjblue rwjblue merged commit 85498f7 into broccolijs:master Mar 11, 2020
@rwjblue rwjblue changed the title lookupDestinationPath will handle relative path Ensure lookupDestinationPath will handle relative path May 9, 2020
@rwjblue rwjblue added the bug label May 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants