Rails 3.0.0 creates directories in --pretend mode [ minor bug ]

@Rohit

Not sure if i understood your comment correctly, but i would imagine, that in -p pretend mode, not just .gitignore but NO directories should be created. Yes?

@tilo, if you are interested in writing a patch, awesome, if not, i don't mind going ahead and doing it. I need the practice :)

@Aditya

The OP mentioned .gitignore file and other things.

let's assume the XYZ Rails project folder already exists, e.g. it contains old git repo with older Rails on a different branch

mkdir XYZ ls -a XYZ . .. .git .gitignore

However you don't need all that to reproduce this issue. That's what I was pointing out. :)

From my initial investigation it seems like the problem happens when the generator action has a combination of empty_directory "config" followed by a inside "config" block call.

    def log
      empty_directory "log"

      inside "log" do
        %w( server production development test ).each do |file|
          create_file "#{file}.log"
          chmod "#{file}.log", 0666, :verbose => false
        end
      end
    end

I'll try to investigate if this is indeed the right way to write the action and its a thor bug or we should be writing the action differently.
The app test should also be fixed to ensure that it fails properly, but i've detected that location already.

I just investigated a little bit and it's possible that this is a Thor issue. Although I'm not sure, do give a look into Thor in case you can't find the bug in Rails source.

Aditya, I think you've found the problem. In Thor — lib/thor/actions.rb

    def inside(dir='', config={}, &block)
      verbose = config.fetch(:verbose, false)

      say_status :inside, dir, verbose
      shell.padding += 1 if verbose
      @destination_stack.push File.expand_path(dir, destination_root)

-->      FileUtils.mkdir_p(destination_root) unless File.exist?(destination_root)
      FileUtils.cd(destination_root) { block.arity == 1 ? yield(destination_root) : yield }

      @destination_stack.pop
      shell.padding -= 1 if verbose
    end

The line with --> is creating the directory as it's not checking for pretend. If you do check for pretend by appending || options[:pretend] then the next line fails (obviously) as there is no dir.

@rohit yes i now believe it should be a thor bug. In the pretend mode, i believe the "inside" method does not respect the pretend flag.

    def inside(dir='', config={}, &block)
      verbose = config.fetch(:verbose, false)

      say_status :inside, dir, verbose
      shell.padding += 1 if verbose
      @destination_stack.push File.expand_path(dir, destination_root)

      FileUtils.mkdir_p(destination_root) unless File.exist?(destination_root) # <--- what about pretend??
      FileUtils.cd(destination_root) { block.arity == 1 ? yield(destination_root) : yield }

      @destination_stack.pop
      shell.padding -= 1 if verbose
    end

I suppose the next step would be to wait for a Thor response (i've created a bug http://github.com/wycats/thor/issues/issue/74 there), but this shouldn't stop us from checking in a failing test meanwhile? Rails CI would start popping up with failures right? What the general rule around this?

i should check my email and refresh before posting. we're literally working on the "same lines". :)

Aditya, yeah me too! :P

Ok good news is that it's definitely a Thor bug. Confirmed this with Jose. However he is too busy to work on this. Can you try and write the patch for Thor? The main thing to keep in mind is that in the pretend mode, the messages like —

      create  config/locales
      create  config/locales/en.yml
      create  config/boot.rb
      create  config/database.yml

Should still appear. So Jose has suggested that in pretend mode, you create the directory, pretend to create the files and then remove the directory.

In any case append the above information regarding the inside method to the Thor ticket and continue discussing the issue there. I'll leave this ticket open till the issue is resolved in Thor.

Also I think you can go ahead with the failing test for Rails too. :)

Alrighty, thor patch sent with tests (http://github.com/wycats/thor/pull/75)

Working on the failing test. Seem like assert_no_file is acting real fishy!

David,

I agree with you completely but since i'm kinda new to this, i started investigating if there was already a test in Rails to test the --pretend functionality or not.

I found that there is.

  def test_application_generate_pretend
    run_generator ["testapp", "--pretend"]
    DEFAULT_APP_FILES.each{ |path| assert_no_file path }
  end

Had a look at DEFAULT_APP_FILES and wondered why it wasn't failing.
Discovered it didnt list "config" and "tmp". Fair enough, now we really dont need to test for that since we'll kinda make up for it in thor.
BUT, it DID list "log" and "log" was being created and left around, so WHY was this test "assert_no_file path" passing?

I think this test is broken. It was testing for the wrong files in the wrong place, passing and thus giving a false positive.

It needs to be to really work

  def test_application_generate_pretend
    run_generator ["testapp", "--pretend"]
    DEFAULT_APP_FILES.each{ |path| assert_no_file File.join("testapp",path) }
  end

and here is the failing test for rails patch. Applying this will make the rails tests fail correctly until the next release of thor.

I don't get it. assert_no_file uses destination_root just like assert_file, so it shouldn't be necessary to use File.join. I think it would be desirable to get to the root of this issue and make assert_no_file work properly.

  def test_invalid_application_name_is_fixed
    run_generator [File.join(destination_root, "things-43")]
    assert_file "things-43/config/environment.rb", /Things43::Application\.initialize!/
    assert_file "things-43/config/application.rb", /^module Things43$/
  end

There are other examples too, but i think if a parameter is provided to run_generator, then it must also be prefixed before checking for the file.

In the test case i fixed, "testapp" is being provided as a parameter, but the test happens without it. Thus it needs to be fixed. I see other example in app_generator_test.rb which all assert_file with app name prefixed.

David, even if the current test was working, adding new tests is always a good choice. Remember that, in the future, we decide to not use thor anymore, we need to have a reliable and consistent test suite in Rails.

Aditya, I am still pondering what you said that pretend should not do anything (as some filesystem may not even have permissions for operation). That said, I believe that the correct fix in Thor is to simply not create the directory.

If should be something like this:

    if options[:pretend]
      yield(destination_root)
    else
      FileUtils.cd(destination_root) { yield(destination_root) }
    end

Do you think you can try a new thor patch and tell us if the result still work?

Sure José, I'll try your suggestion out in thor and send a pull request there like last time.

Jose,

You've now fixed thor not to generate stuff in the pretend mode. We should based on your comments above also add the tests to rails when we make rails depend on the latest version of thor.

Automatic cleanup of spam.

@Aditya Sanghi Yes and Rails already depends on latest thor. That said, do you think you can provide a test case? Thanks!

@jose, i've added the test in the patch above already.

There was a test for pretend mode already but it was giving a false positive. I've fixed that in the patch attached to this ticket.

Are you referring to some other test that i've not understood, perhaps?

Sorry, I have missed this patch as well. I will apply it later! Thanks!

Gentle Bump! Perhaps push this to master before its forgotten?