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

Duplicate prompt for all cmder/cmd shells (Powerline, etc. as well) #1882

Closed
CollinChaffin opened this issue Sep 16, 2018 · 14 comments
Closed

Comments

@CollinChaffin
Copy link

CollinChaffin commented Sep 16, 2018

As of the last couple commits, there is a major error in clink.lua that causes all prompts in standard cmd prompts such as powerline etc. to be duplicated as shown here:

snag_9-15-2018_21-09-30

This took me hours to track down exactly which file was the cause, through trial and error going back to July's 6f10db0 commmit which I can confirm is the last known good (that I have locally) that does not exhibit this behavior. If you simply use the clink.lua from that commit, the prompts return to normal as shown here:

snag_9-15-2018_21-09-52

From diffing that known good and the latest b61c4a2 commit's clink.lua, there are quite a few changes so I'll try in-between things here to narrow it down but wanted to get this posted as I did not see it already and like I said this was a real PITA to suddenly have all cmder prompts doubled and it not be in the build/compile, but in one of the many ancillary support files.

@CollinChaffin
Copy link
Author

Also, the new addition of lines 378+ that load all the LUA files in CONFIG with this new behavior: "-- Skip files that starts with _. This could be useful if some files should be ignored" is a problem since addons such as the very popular Powerline cmd prompt's only primary config LUA that needs to be executed itself begins with an underscore (FILENAME: _powerline_config.lua) and I believe is a show-stopper if it's config isn't read in properly so you may want to reconsider the "ignore" character, if one is really necessary.

From what I can tell this is not the root cause of this duplication thought and I am still debugging. At first glance, it sure looked like the basically duplicated blocks I just mentioned at end of file lines (lines 378 to EOF) sure looked to me like it could result also in duplication of LUA execution depending on how one's config env variables are set. However, in my case when I tested I obviously had to have CMDER_ROOT set so the first block setting cmder_config_dir is properly setting to cmder/config, but did not have CMDER_USER_CONFIG defined so that obviously is not the cause so I am still debugging but with all these calls through batch files and LUA files, it's been about 30 yrs since I've had to follow this many batch calls/goto/gosubs LOL. :)

@CollinChaffin
Copy link
Author

Okay so I have confirmed my initial thought above was actually correct and is the root cause.

This new code block is causing duplicated prompts:

for _,lua_module in ipairs(clink.find_files(cmder_config_dir..'*.lua')) do
    -- Skip files that starts with _. This could be useful if some files should be ignored
    if not string.match(lua_module, '^_.*') then
        local filename = cmder_config_dir..lua_module
        -- use dofile instead of require because require caches loaded modules
        -- so config reloading using Alt-Q won't reload updated modules.
        dofile(filename)
    end
end

Simply block-commenting this out (for now) resolves all issues. Since this block was just added and never present before, I would ask for clarification as to the reasoning for adding it now and I would be happy to help to come up with a solution for whatever that new need is, but it is clear as-is that it is adding duplicating read behavior that should probably be avoided in any case.

@daxgames
Copy link
Member

@CollinChaffin the last two blocks were added in order to load user lua files at the end of processing the clink.lua. The idea is to allow users to have their own lua customization capability without having to edit the clink.lua file since it is a core piece of cmder and is subject to overwrite in update. Changing any file inside the vendor folder is be discouraged because data low can occur in upgrad if the user forgets to backup his/her changes.

Have you edited the clink.lua? If so I suggest moving your edits to one or more lua files in the config folder.

The last two blocks are very similar and they are there to support cmder ability to have a shared config cmder_config_dir for running cmder from a shared location and also allowing users to add their own config 'cmder_user_config_dir'.

I did some testing and it looks like the block you commented is actually unnecessary. Files in this folder are being loaded by clink.lua and then again by clink.exe, I think.

I commented it and lua in the config folder is being loaded by clink directly. So it looks like you found a bug. I think I know how best to fix it so we maintain full functionality. I will do that today.

Can you point me to how you are adding power line so I can actually test the fix?

The ignoring lua files that begin with _ came from the block above where clink_completions modules are included. I don't really have any issues with changing the ignore prefix, I doubt it is a widely used feature of cmder.

If we change the ignore prefix I suggest we change it for all cases though.

@cmderdev/trusted-contributors any issues with this?

@daxgames
Copy link
Member

Also in my testing the ignore code is not working anyway so it could probably just be removed.

@daxgames
Copy link
Member

daxgames commented Sep 16, 2018

@CollinChaffin - Are you using cmder-powerline-prompt

@CollinChaffin
Copy link
Author

No, I have not the block above was added in the last couple commits here and is the cause of the issues. I never edit any lua files outside of the CONFIG folder. Upon upgrading to the last 2 commits (# above), those 2 new blocks appeared in the CLINK.LUA provided HERE in cmder.z. I'll post in a min to show you exactly which commit they first appeared in - but they are the cause - not any edit I've made. Unzipped new cmder.z from here - and broken until I spent hours to figure out why.

@CollinChaffin
Copy link
Author

I'm confused the root cause is right there like I said when clink.lua was changed on commit even titled "run user lua afer cmder lua" e69e7f9.

Prior to that, the offending code that is causing duplicate loads is NOT there - I just re-confirmed. Obviously, before that commit, user LUAs were ALREADY being loaded correctly from CONFIG folder - I know because we all place our LUAs there to load for years. The issue here is that in adding this offending code commit, you did not remove the prior LUA load code (wherever the heck it is these batch/lua files are driving me nuts) and now you've added a SECOND call to loop through them and load user LUAs. What I'm loading is irrelevant - the code is calling LUA loads twice and since you just said those blocks could be removed, it sounds like maybe you don't know why you added them in that commit?

The exact diff is right here showing it was added on Sept 1st:

snag_9-16-2018_08-21-13

And if you pull repo on commit 0855075 prior to it and unzip clink.lua, the offending blocks are not present.

@CollinChaffin
Copy link
Author

CollinChaffin commented Sep 16, 2018

Now I can help you easily figure out the best path because I'm guessing you were trying on Sept 1st in those code blocks to better ensure that USER luas were being properly loaded with some exceptions. So, if I pull the repo in July prior to that commit, unzip ONLY that clink.lua and replace the broken one from the Sept 1st commit, the million dollar question is that will greatly help me from having to sit and step through LUA and batch files all day is - all my LUA files in the CONFIG folder are still ALL being loaded properly (and always were) - so in which file does the code reside that is loading CONFIG directory LUA files - since those 2 offending code blocks that also do LUA loading are now not present at all?

EDIT: Forgot to answer not that it matters since this affects much more than just that prompt, but yes, that is cmder-powerline-prompt.

@daxgames
Copy link
Member

@CollinChaffin I am not arguing with you, In fact I agree with you 100%. You are not understanding my last post. As I said earlier you found a bug, it happens, but the solution is not to just remove both code blocks.

The below explains why:

If a user launches cmder.exe /c %CMDER_USER_CONFIG% then all the user config including lua is loaded from %CMDER_USER_CONFIG%\config after loading %cmder_root%/config/user_profile.cmd and %cmder_root%/config/profile.d/*.cmd. However in this case lua is not automatically loaded from %cmder_root%/config because the clink profile is set to %CMDER_USER_CONFIG%/config. Clink itself loads lua from the clink profile folder, I am not sure if there is any specific lua code responsible for it.

The desired effect is load all 'shared/team' config from %cmder_root%/config and then 'user specific' config from %CMDER_USER_CONFIG%/config so teams can share a cmder config and still have user specific config.

The code in clink.lua should therefor be:

if clink.get_env('CMDER_USER_CONFIG') then
  local cmder_config_dir = clink.get_env('CMDER_ROOT')..'/config/'
  for _,lua_module in ipairs(clink.find_files(cmder_config_dir..'*.lua')) do
    local filename = cmder_config_dir..lua_module
    -- use dofile instead of require because require caches loaded modules
    -- so config reloading using Alt-Q won't reload updated modules.
    dofile(filename)
  end
end

So that if %CMDER_USER_CONFIG% is set clink also loads config from %cmder_root%/config.

Notice I also removed the ignore code based on your feedback.

Hopefully this makes sense.

@daxgames daxgames mentioned this issue Sep 16, 2018
@daxgames
Copy link
Member

@CollinChaffin PR #1884 submitted please test.

@daxgames
Copy link
Member

I have tried #1884 with and without the /c [path] argument and I am not getting duplicate prompts with cmder-powerline-prompt

@daxgames
Copy link
Member

daxgames commented Sep 16, 2018

@CollinChaffin Hey just curious does the powerline git aware prompt work for you? It does not seem to for me. EDIT: I got it working, I had another git_prompt.lua that was breaking it.

@CollinChaffin
Copy link
Author

CollinChaffin commented Oct 13, 2018

I'm so sorry I missed this one I just pulled down the latest from appveyor to re-test and will update shortly!

EDIT: I'm late to the game but your #1884 fix I can confirm looks like it did resolve the issue. Thanks so much for getting to it so quickly and I'm sorry if I came across argumentative sometimes I feel I'm not doing the best job of explaining so I tend to then throw too much info in. :)

@daxgames
Copy link
Member

daxgames commented Oct 13, 2018

@CollinChaffin No probmem. Thanks for the bug report.

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