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

Do not put current directory in LOAD_PATH #114

Closed
mainameiz opened this issue Mar 20, 2019 · 14 comments
Closed

Do not put current directory in LOAD_PATH #114

mainameiz opened this issue Mar 20, 2019 · 14 comments

Comments

@mainameiz
Copy link
Contributor

Hello everybody!
First of all, thanks for making such a good project. It is a great addition to such a low-level ruby-kafka :)


My question...

I have found a strange issue using racecar + bootsnap.

As you may know Bootsnap recursively scans everything in $LOAD_PATH and caches all paths to speed up application boot process.

Also I have found that racecar adds current directory to $LOAD_PATH - 0849ecc.

This causes bootsnap to scan every directory in my rails project :) including shared storage linked to public/uploads.

So my question: is it necessary to put current directory into $LOAD_PATH?

@mainameiz
Copy link
Contributor Author

I also make a PR #115

@dasch
Copy link
Collaborator

dasch commented Apr 2, 2019

It's done in order for --require to work, I believe.

@mainameiz
Copy link
Contributor Author

Oh, I understand. May be this can be done without loading current directory to $LOAD_PATH?

E.g.

        opts.on("-r", "--require STRING", "Require a library before starting the consumer") do |lib|
          begin
            require lib
          rescue LoadError => _exception
            require File.join(Dir.pwd, lib)
          end
        end

What do you think?

@dasch
Copy link
Collaborator

dasch commented Apr 2, 2019

I think that will subtly change how Ruby loads files, which may cause files to be loaded twice. The string passed to require is used to check if a file has already been loaded.

@dasch
Copy link
Collaborator

dasch commented Apr 2, 2019

Does Rails not also put the current dir in the load path? Or is it just lib/?

@dasch
Copy link
Collaborator

dasch commented Apr 2, 2019

I think one way to deal with this would be to only add to the load pass if --require has been passed.

@mainameiz
Copy link
Contributor Author

Does Rails not also put the current dir in the load path? Or is it just lib/?

Yes, just /lib is placed in $load_path, but not root directory.

@dasch
Copy link
Collaborator

dasch commented Apr 2, 2019

It would be a breaking change to change from current dir to ./lib/, so I don't think that's an option – but if the modification of the load path can be made conditional on one or more --require args being passed then all can be happy?

@mainameiz
Copy link
Contributor Author

I think one way to deal with this would be to only add to the load pass if --require has been passed.

Not quite understand. Do you mean this?

        opts.on("-r", "--require STRING", "Require a library before starting the consumer") do |lib|
          begin
            require lib
          rescue LoadError => _exception
            $LOAD_PATH.unshift(Dir.pwd)
            require lib
          end
        end

@mainameiz
Copy link
Contributor Author

Or?

        opts.on("-r", "--require STRING", "Require a library before starting the consumer") do |lib|
            $LOAD_PATH.unshift(Dir.pwd)
            require lib
        end

@dasch
Copy link
Collaborator

dasch commented Apr 2, 2019

Not quite. More like

load_path_modified = false

# ...

        opts.on("-r", "--require STRING", "Require a library before starting the consumer") do |lib|
            $LOAD_PATH.unshift(Dir.pwd) unless load_path_modified
            load_path_modified = true
            require lib
        end

@mainameiz
Copy link
Contributor Author

🤔 why we need load_path_modified? Whether Racecar::Cli can be called multiple times?

@dasch
Copy link
Collaborator

dasch commented Apr 2, 2019

You can require multiple files, e.g. --require foo --require bar. The block will be executed each time.

@mainameiz
Copy link
Contributor Author

mainameiz commented Apr 2, 2019

Your suggested solution - #117

@dasch dasch closed this as completed Apr 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants