Skip to content
This repository has been archived by the owner on Jan 2, 2023. It is now read-only.

Fixed minor bug + improved experience when folder/file not present #10

Closed
wants to merge 1 commit into from

Conversation

sivsushruth
Copy link
Contributor

The require "stringio" was placed after it was actually called. Hence it threw an error saying "NameError: uninitialized constant Puck::DependencyResolver::StringIO".
Also added a few lines of code to explicitly let the user know that the bin or the lib file was not created.

The require "stringio" was placed after it was actually called. Hence it
threw an error saying "NameError: uninitialized constant
Puck::DependencyResolver::StringIO".
Also added a few lines of code to explicitly let the user know that the
bin or the lib file was not created.
@iconara
Copy link
Owner

iconara commented Feb 3, 2015

@sivsushruth thanks for the fix

@grddev could you review the StringIO thing?

@@ -41,7 +42,6 @@ def contained_bundler(gem_home, gemfile, lockfile, groups)
begin
line = __LINE__ + 1 # as __LINE__ represents next statement line i JRuby, and that becomes difficult to offset
unit = scripting_container.parse(StringIO.new(<<-"EOS").to_inputstream, __FILE__, line)
require 'stringio'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think StringIO is needed in both places (since here it's run from another ScriptingContainer), but weirdly the tests pass like this too (they passed before too, though).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this line isn't needed anymore. Once it was used inside the scripting container to avoid errors being written to stderr. But the current implementation catches the errors, transformas them into "raw data" and rethrows them from outside the scripting container.

The outer require should be added as we have a direct dependency on stringio. Technically, it is only needed if bundler isn'n loaded, as it should be fulfilled by the following dependency chain bundler/source_list -> bundler/source/rubygems -> net/http -> stringio, which is autoloaded whenever a bundler definition is parsed. I'm not sure if we could easily (or if we even want to) run the specs without loading bundler.

Assuming that this is the explanation, it should be able to run bundle exec puck instead of just puck as a workaround (or adding the missing require as done in this PR).

@iconara
Copy link
Owner

iconara commented Feb 3, 2015

@sivsushruth see my inline comment about the directory check. You could either amend this PR with some tests for the feature or we can treat it as a bug report and fix that when we get time.

@sivsushruth
Copy link
Contributor Author

@iconara i will amend the PR

@sivsushruth
Copy link
Contributor Author

@iconara I first ran puck without the bin folder and it threw an error, so I do not understand why we should accept bin not being there ?

@iconara
Copy link
Owner

iconara commented Feb 3, 2015

If your project doesn't have any bin files you can still run bin files from other gems, so not having a bin directory should be allowed. Not having a lib directory is trickier, but I guess that could be allowed too, you could make the same argument there: if someone wants to package a jar that basically only brings together a bunch of gems that should work too.

But maybe I misunderstood your objection: what I mean is that Puck shouldn't throw any error, the bug is that it throws an error, not that it doesn't check for a bin dir.

@iconara
Copy link
Owner

iconara commented Feb 5, 2015

I've merged the StringIO part of this in 2fb2ab4.

@iconara iconara added this to the v1.0.0 milestone Feb 5, 2015
@iconara
Copy link
Owner

iconara commented Feb 12, 2015

I'm closing this because of 04fa4ce

@iconara iconara closed this Feb 12, 2015
@sivsushruth
Copy link
Contributor Author

👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants