Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

Comments

~1000 redundant lines removed from mak/WINDOWS#3024

Closed
denizzzka wants to merge 5 commits intodlang:masterfrom
denizzzka:wildcards
Closed

~1000 redundant lines removed from mak/WINDOWS#3024
denizzzka wants to merge 5 commits intodlang:masterfrom
denizzzka:wildcards

Conversation

@denizzzka
Copy link
Contributor

No description provided.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @denizzzka! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + druntime#3024"

Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

Looks great, ping me when its good.

@Geod24
Copy link
Member

Geod24 commented Apr 10, 2020

Looks like the right direction, however I'd ping some people that are familiar with this build system before merging. Also can we not add more top-level file ? Put them in mak for example.

@denizzzka
Copy link
Contributor Author

Also can we not add more top-level file ? Put them in mak for example.

I suggest first to decide exactly where to move it. Because it looks like ./mak/ directory is for makefiles strictly.

@denizzzka
Copy link
Contributor Author

@thewilsonator ping :-)

@thewilsonator
Copy link
Contributor

Please put those .bat files in mak

@denizzzka
Copy link
Contributor Author

@thewilsonator done!

set WORKING_DIR=%cd%
set ROUTINE=%cd%\mak\wroutine.bat

C:\Windows\System32\forfiles.exe /p %PATH% /m %MASK% /s /c "cmd /c %ROUTINE% @RELPATH"
Copy link
Member

Choose a reason for hiding this comment

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

Don't use the absolute path here. I would expect c:\Windows\System32 to be in PATH. Even if not, c:\Windows is not guaranteed to be the installation path, %windir% contains it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first I tried it without full path, but it works only in local console, but not in .bat files. I don't know why. If you can please fix it.

@Geod24
Copy link
Member

Geod24 commented Apr 10, 2020

@thewilsonator : I explicitly asked to ping people familiar with the build system before mindlessly merging this.

For example, doing a bit of git blame on those files would have pointed you to this PR which explicitly reverts some of the changes that were done here.

CC @rainers .

@rainers
Copy link
Member

rainers commented Apr 10, 2020

If we use wildcard processing, why not just use xcopy?

Does anyone remember what's special about the modules in the core/sync folder? The generated di files still contain platform specific code (the ctors are not reduced to the declaration !?), so why are they not simply copied aswell?

@denizzzka
Copy link
Contributor Author

denizzzka commented Apr 10, 2020

@rainers I just replaced lines of code by same but generated. I have no idea why it was implemented by this way.

@Geod24

before mindlessly merging this.

Ditto. This PR changes nothing except replace of tons of copy-pasted code.
Any related changes can be done after this PR.

(I did this PR not because I suddenly decided to fall in love with Windows :-) But because this huge mak/WINDOWS file prevents me from making other changes that aren't related to Windows.)

@rainers
Copy link
Member

rainers commented Apr 10, 2020

I don't like the verbosity of the makefiles, but I'm afraid that this PR adds too many subtle details and brittle build steps. Walter is rather fond of the explicit list of commands.
If we have a host compiler with phobos available, most of mak\WINDOWS can be replaced by

copy: generated\copyimports.exe $(COPY)
	generated\copyimports.exe $(COPY)

generated\copyimports.exe: mak\copyimports.d
	$(DMD) -of=$@ $**

with this tiny D tool:

module copyimports;
import std.array, std.file, std.path, std.stdio;

void main(string[] args)
{
    string importPath = absolutePath("import");
    string srcPath = absolutePath("src");
    foreach(impfile; args[1..$])
    {
        impfile = absolutePath(impfile);
        string srcfile = buildPath(srcPath, asRelativePath(impfile, importPath).array);
        if (std.file.exists(impfile))
            if (timeLastModified(impfile) >= timeLastModified(srcfile))
                continue;
        writeln("copying ", srcfile, " -> ", impfile);
        mkdirRecurse(dirName(impfile));
        std.file.copy(srcfile, impfile);
    }
}

@denizzzka
Copy link
Contributor Author

denizzzka commented Apr 10, 2020

Can anyone from WIndows World implement @rainers proposal?

@12345swordy
Copy link
Contributor

@denizzzka
Copy link
Contributor Author

denizzzka commented Apr 10, 2020

@12345swordy I am personally currently not have problem with Windows VM. But stil, this is alien system which is not my "goal".

I look at it like this:

By some coincidence, current mak/WINDOWS affects all systems (any addition/deletion of .d file requires changes to it). I fixed it by this PR.

It would be fair to accept the patch as is and then, people interested in the Windows platform will abble fix it or do something else. Non-Windows people will be able to do things as they see fit without paying attention to mak/WINDOWS.

Otherwise, this situation will continue rot for years.

@rainers
Copy link
Member

rainers commented Apr 11, 2020

Can anyone from WIndows World implement @rainers proposal?

See #3026

@denizzzka
Copy link
Contributor Author

#3026 concluded as better solution

@denizzzka denizzzka closed this Apr 20, 2020
@denizzzka denizzzka deleted the wildcards branch November 22, 2022 14:30
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.

6 participants