Skip to content

Comments

move res/ subdirectory to src/dmd/res/#11269

Merged
dlang-bot merged 1 commit intodlang:masterfrom
WalterBright:mv-res
Jun 15, 2020
Merged

move res/ subdirectory to src/dmd/res/#11269
dlang-bot merged 1 commit intodlang:masterfrom
WalterBright:mv-res

Conversation

@WalterBright
Copy link
Member

Builds should not reference files up and out of the build directory and to the left.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

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 + dmd#11269"

@WalterBright WalterBright force-pushed the mv-res branch 3 times, most recently from 6aea466 to 69f6d92 Compare June 14, 2020 05:19
@WalterBright
Copy link
Member Author

CyberShadow/DAutoTest says:

Invalid source/import path: /dev/shm/dtest/work/repo/dmd/res

where is that path specified?

@Geod24
Copy link
Member

Geod24 commented Jun 14, 2020

dmd/dub.sdl

Line 74 in ca636dc

stringImportPaths "res"

@WalterBright
Copy link
Member Author

Thanks @Geod24 as res is not greppable given it appears everywhere. I never would have found it on my own.

@Geod24
Copy link
Member

Geod24 commented Jun 15, 2020

Builds should not reference files up and out of the build directory and to the left.

Alternatively, I would say that build files should not be mixed with source files. Having makefiles and build.d in src has always irked me, but I think that's too late to change.

@WalterBright
Copy link
Member Author

Thanks @Geod24 but there appears to be yet another one:

../dmd/generated/linux/release/64/dmd -J../dmd/res -J../dmd/generated/linux/release/64/ -c -o- -version=MARS -version=CoreDdoc \
	-version=StdDdoc -Df.generated/.prerelease-dummy.html \
	-Xf.generated/docs-prerelease.json -I../phobos @.generated/.prerelease-files.txt
../dmd/src/dmd/doc.d(364): Error: file `"default_ddoc_theme.ddoc"` cannot be found or not in a path specified with `-J`

@kubo39
Copy link
Contributor

kubo39 commented Jun 15, 2020

It seems unrelated to this failure, but I found one more case in ci.sh:test_dub_package.

"${build_path}/dmd" -version=NoBackend -version=GC -version=NoMain -Jgenerated/dub -Jres -Isrc -i -run test/dub_package/frontend.d

@WalterBright
Copy link
Member Author

@kubo39 thanks, but where is that file? It's not in the dmd repository.

@kubo39
Copy link
Contributor

kubo39 commented Jun 15, 2020

@WalterBright
here:

dmd/ci.sh

Line 118 in 6ee0456

"${build_path}/dmd" -version=NoBackend -version=GC -version=NoMain -Jgenerated/dub -Jres -Isrc -i -run test/dub_package/frontend.d

@WalterBright
Copy link
Member Author

thanks, trying it now

@WalterBright
Copy link
Member Author

same failure :-(

@kubo39
Copy link
Contributor

kubo39 commented Jun 15, 2020

@WalterBright
Copy link
Member Author

Thank you. Blocked by dlang/dlang.org#2821

targetType "library"
sourcePaths "src/dmd"
stringImportPaths "res"
stringImportPaths "src/dmd/res"
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to merge dlang.org PR without duplication, you need to temporarily add both directories here as dlang.org uses dmd as a library:

stringImportPaths "res"
stringImportPaths "src/dmd/res"

<DCompile>
<VersionIdentifiers>MARS</VersionIdentifiers>
<StringImportPaths>..\..\res;$(OutDir)</StringImportPaths>
<StringImportPaths>../dmd/res;$(OutDir)</StringImportPaths>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<StringImportPaths>../dmd/res;$(OutDir)</StringImportPaths>
<StringImportPaths>..\dmd\res;$(OutDir)</StringImportPaths>

(similar for the changes below)

This is the Visual Studio project file, so I am pretty sure the Windows slashes should be kept.

@wilzbach
Copy link
Contributor

Having makefiles and build.d in src has always irked me, but I think that's too late to change.

You're not only there. However, build.d isn't called directly by any CI (or packagers) yet, but only via the Makefiles.
So it would be possible to move build.d up to the root-level, adjust its paths and change the paths to build.d in the makefiles.
For full compatibility the src/build.d could print a deprecation message and execute sth. along: $DC -run <scriptDir>/../build.d <args>.

wilzbach added a commit to wilzbach/dmd that referenced this pull request Jun 15, 2020
@MoonlightSentinel
Copy link
Contributor

You're not only there.

Ditto

However, build.d isn't called directly by any CI (or packagers) yet, but only via the Makefiles.

./dmd/src/build.d -j2 MODEL=64
make -C druntime -f posix.mak -j2 MODEL=64
make -C phobos -f posix.mak -j2 MODEL=64
# Both version can live side by side (they end up in a different directory)
# However, since clang does not provide a multilib package, only test 32 bits with g++
if [ ${{ matrix.compiler }} == "g++" ]; then
./dmd/src/build.d install -j2 MODEL=32

dmd/.circleci/run.sh

Lines 181 to 186 in d263f80

./src/build.d clean
rm -rf generated # just to be sure
# TODO: add support for 32-bit builds
./src/build.d MODEL=64
./generated/linux/release/64/dmd --version | grep -v "dirty"
./src/build.d clean

@wilzbach
Copy link
Contributor

However, build.d isn't called directly by any CI (or packagers) yet, but only via the Makefiles.

Oh Thanks for the correction @MoonlightSentinel. I wasn't aware that we were already doing this, but as these files are still in the same repo + under our control, I guess it's a now or never decision, because I believe packagers will soon switch to use src/build.d directly and we want to make their version bumps as smooth as possible.

@wilzbach
Copy link
Contributor

(restarted all CIs by force-pushing)

@wilzbach
Copy link
Contributor


[13] [14] (/usr/share/texlive/texmf-dist/tex/latex/base/ts1cmtt.fd)
! You can't use `\spacefactor' in vertical mode.
\add@accent ...}\accent #1 #2\egroup \spacefactor 
                                                  \accent@spacefactor 
l.592     \t
            
? 
! Emergency stop.
\add@accent ...}\accent #1 #2\egroup \spacefactor 
                                                  \accent@spacefactor 
l.592     \t
            
!  ==> Fatal error occurred, no output PDF file produced!
Transcript written on .generated/dlangspec.log.
posix.mak:553: recipe for target '/dev/shm/dtest/work/repo/dlang.org/web/dlangspec.pdf' failed

@CyberShadow any ideas? I wasn't able to reproduce this locally. dlangspec.tex is the same for me with/without this PR.

@CyberShadow
Copy link
Member

Oh, no. Not another PDF problem. 😞

I don't know what happened there. A random fluke?

@CyberShadow
Copy link
Member

The cure: dlang/dlang.org#2823

@dlang-bot dlang-bot merged commit fe36075 into dlang:master Jun 15, 2020
@WalterBright
Copy link
Member Author

Thanks everyone for the help with this! There are more files like this that need fixing, but I'm a bit battle fatigued at the moment and will do them later.

@WalterBright WalterBright deleted the mv-res branch June 15, 2020 23:36
MoonlightSentinel added a commit to MoonlightSentinel/dmd that referenced this pull request Jan 21, 2021
This ensures the resulting path is correct even for the sources
distributed alongside the official releases.

This is a regression introduced by dlang#11269.
dlang-bot pushed a commit that referenced this pull request Jan 21, 2021
This ensures the resulting path is correct even for the sources
distributed alongside the official releases.

This is a regression introduced by #11269.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants