latex3 / l3build

A testing and building system for LaTeX
LaTeX Project Public License v1.3c
84 stars 14 forks source link

Clean target creates build directory #369

Closed jlaurens closed 1 week ago

jlaurens commented 2 weeks ago

From the examples

pushd examples/Simple-Flat
ls build
l3build clean
ls build
popd

The output is

ls: build: No such file or directory
distrib     doc     local       test        unpacked

This causes a problem with bundles on the second clean

pushd examples/Bundle-Flat
l3build clean
l3build clean
popd

The output reads

Running l3build with target "clean" for module "Module-One"
Running l3build with target "clean" for module "Module-Two"
Running l3build with target "clean" for module "Module-One"
Running l3build with target "clean" for module "Module-Two"
Running l3build with target "clean" for module "build"
Error: Cannot find configuration build.lua
josephwright commented 2 weeks ago

I see why that's happening and why it doesn't show up for https://github.com/latex3/latex2e/ and https://github.com/latex3/latex3/ - I'll take a look.

davidcarlisle commented 2 weeks ago

@josephwright one possible fix would be to make the default list of modules be every directory that has a build.lua, rather than every directory:

function listmodules()
  local modules = { }
  local exclmodules = exclmodules or { }
  for entry in lfs.dir(".") do
    if entry ~= "." and entry ~= ".." then
      local attr = lfs.attributes(entry)
      assert(type(attr) == "table")
      if attr.mode == "directory" and fileexists(entry .."/" .."build.lua") then
        if not exclmodules[entry] then
          insert(modules, entry)
        end
      end
    end
  end
  return modules
end
josephwright commented 2 weeks ago

@davidcarlisle Yes, that's exactly what I was thinking :)

josephwright commented 2 weeks ago

@davidcarlisle Adding

exclmodules[(gsub(builddir,"^%./",""))] = true

seems to do it - make sense?

davidcarlisle commented 2 weeks ago

@josephwright I wondered about using exclmodules, but then if the user does use that it's up to them to include build,either works i suppose (or both:-)

jlaurens commented 2 weeks ago

Posted a PR

jlaurens commented 2 weeks ago

Anyways, the clean target still creates directories and this has also to be fixed.

josephwright commented 2 weeks ago

Anyways, the clean target still creates directories and this has also to be fixed.

Yes, of course it does: clean should leave the directory in a 'ready' state, and that includes having the build dir present-but-empty.

jlaurens commented 2 weeks ago

Well this is not really what is documented.

A --deep option could be used, in which case no directory would be created.

BTW, the exclmodule business must take into account all the directories that are created by the clean target, especially on customization. For example, if builddir is foo/bar, then only foo should be considered. The same for the other directories.

josephwright commented 2 weeks ago

Well this is not really what is documented.

A --deep option could be used, in which case no directory would be created.

I'm clearly missing something here: what's the issue with the current approach? As l3build is in use, the directory used for 'construction' has to exist for anything to get done.

BTW, the exclmodule business must take into account all the directories that are created by the clean target, especially on customization. For example, if builddir is foo/bar, then only foo should be considered. The same for the other directories.

I'm not sure quite what you mean - an example structure?

jlaurens commented 2 weeks ago

Well this is not really what is documented. A --deep option could be used, in which case no directory would be created.

I'm clearly missing something here: what's the issue with the current approach? As l3build is in use, the directory used for 'construction' has to exist for anything to get done.

The problem is outside of l3build. For example, if you want to automatically compare 2 copies of the same package, one vanilla readonly base copy and a working copy. Then cleaning the working copy makes it different despite nothing was done at all.

BTW, the exclmodule business must take into account all the directories that are created by the clean target, especially on customization. For example, if builddir is foo/bar, then only foo should be considered. The same for the other directories.

I'm not sure quite what you mean - an example structure?

In Bundle-Flat add builddir="foo/bar" to build.lua, then when cleaning, instead of having the standard

Running l3build with target "clean" for module "build"

you have

Running l3build with target "clean" for module "foo"
davidcarlisle commented 2 weeks ago

Running l3build with target "clean" for module "foo"

yes but the simple check I suggested above would avoid that as foo doesn't have a build.lua, I am not sure I see a need for the tree.. search for build.lua that you suggest in the linked PR

josephwright commented 2 weeks ago

Actually, @davidcarlisle's idea is better than mine: avoids any 'in progress' directories until they have a script present.

jlaurens commented 2 weeks ago

@davidcarlisle

I was only talking about the exclmodule business, and this was an argument to follow the listmodules path, notice that this comment was made after I posted the PR.

tree is a glimpse at deeper modules. Just replace "*" by "**" and you allow there submodules at any depth. Of course the other parts of the code must be checked to support deeper modules too.

davidcarlisle commented 2 weeks ago

Sure but I can't see any requirement to add modules at different depths (or even what that would mean) it's not like namespacing in some other languages, the modules in a l3build bundle are just separate ctan submisssions so there isn't any possibility of nesting them, and not a lot of incentive to have them at different depths. But if there was a use case for adding that, it should be brought in as a separate issue not complicating the code in this issue (which ought to have been fairly non controversial as getting an error if calling l3build clean twice is clearly a bug)

josephwright commented 2 weeks ago

I've commited @davidcarlisle's suggestion as it addresses the immediate issue

jlaurens commented 2 weeks ago

So I close the PR, but I do not close the issue because it is not solved.

davidcarlisle commented 2 weeks ago

as @josephwright commented above, having clean leave an empty build directory is by design, so the issue here was getting an error on calling clean twice, not that clean generated the directory.

josephwright commented 2 weeks ago

The clean target is there to make ready for a CTAN run or similar - to make the working directory 'safe' such that a build will not have artefacts from a previous run. There is no need to remove the build dir, and as that is a necessary part of using l3build, making sure it exists makes sense.

jlaurens commented 2 weeks ago

There are 2 issues here

  1. there is a build subtree
  2. there was an error

point 2 has been solved. I perfectly hear that you absolutely don't want to change the clean target. So, what about a clean like action that does not create some directories when they do not exist?

jlaurens commented 2 weeks ago

@davidcarlisle

[...] having clean leave an empty build directory is by design [...]

Isn't everything "by design"? It might be more interesting to know the real motivations for this particular design. I could not find any in the actual documentation, including source code. Any hint?

davidcarlisle commented 2 weeks ago

No. Some things (like having an error on a second clean or some of your other issues) are bugs and definitely not "by design") I was surprised you didn't add mention of creating the build directory in your extended documentation of clean in #371, such a line could still be added, but otherwise I'd say this issue can be closed.

u-fischer commented 2 weeks ago

It might be more interesting to know the real motivations for this particular design.

Joseph already told you that:

clean should leave the directory in a 'ready' state, and that includes having the build dir present-but-empty.

cleaning files is sometimes important to avoid side effects from old files on new checks or saves. cleaning directories is not needed here, and if done would simply slow down the processing as they must be recreated afterwards.

For example, if you want to automatically compare 2 copies of the same package, one vanilla readonly base copy and a working copy. Then cleaning the working copy makes it different despite nothing was done at all.

Then do the comparision as git does it: ignore empty directories.

josephwright commented 2 weeks ago

@davidcarlisle

[...] having clean leave an empty build directory is by design [...]

Isn't everything "by design"? It might be more interesting to know the real motivations for this particular design. I could not find any in the actual documentation, including source code. Any hint?

From memory - cleaning out dirs in a cross-platform way that doesn't lead to output to the terminal was tricky. Forcing an empty dir was easier, and it meant we had a reliably-existing build dir, so that's what I did.

jlaurens commented 2 weeks ago

I see. IMO using rmdir is easier than removing files one by one. Moreover, the existence of build is only reliable when you run l3build just after l3build clean, if you run anything else in between, the existence is not longer guaranteed. Moreover, the documentation states that "A |build/| folder can be safely deleted; all material within is re-generated for each command of the \pkg{l3build} system."

FrankMittelbach commented 1 week ago

Am 30.06.24 um 12:09 schrieb LAURENS Jérôme:

Moreover, the documentation states that "A |build/| folder can be safely deleted; all material within is re-generated for each command of the \pkg{l3build} system."

that statement is true, but it doesn't imply that things are deleted at every run, it only says that if you do that it causes no harm, as in that cases they are recreated (which implies (to me) that the folders aren't automatically deleted each time).