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

Makefiles #24

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from
Draft

Makefiles #24

wants to merge 14 commits into from

Conversation

radonnachie
Copy link
Contributor

I have taken the intvector subexamples and updated their execution from run.sh files to makefiles.

After discussing possible formatting of the makefiles and perhaps its contents, this PR's changes will be expanded to update all examples to a makefile execution.
@umarcor will be tackling the appropriate CI changes.

As of the moment, run.sh files are renamed to old_run.sh files so that the output of the makefiles can be compared.

The first rule in each makefile is the default rule executed, afresh will clean and then run. This is triggered by make

All makefile rules can be called by specifying the target (argument left of the rule's ':') after make: eg make run

make echoes each command before executing it, unless the line starts with @... I have left all lines, excepting echoes, to be echoed as I like the transparency that echoing the commands provides.

Copy link
Member

@umarcor umarcor left a comment

Choose a reason for hiding this comment

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

Thanks for starting this work!

@umarcor will be tackling the appropriate CI changes.

See comment below. Apart from a few very minor issues, this is the only point to be updated/fixed. MSYS2 was broken for a few days, and it took a bit longer than expected to test #25, but it should be good now.

The first rule in each makefile is the default rule executed, afresh will clean and then run. This is triggered by make

All makefile rules can be called by specifying the target (argument left of the rule's ':') after make: eg make run

I think that this fits in the main README.

make echoes each command before executing it, unless the line starts with @... I have left all lines, excepting echoes, to be echoed as I like the transparency that echoing the commands provides.

I like it!

.github/workflows/test.yml Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@@ -0,0 +1,56 @@
CC=gcc
Copy link
Member

Choose a reason for hiding this comment

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

CC?=gcc, GHDL?=ghdl, CFLAGS+=-fPIC, STD?=93, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cant have it all on one line...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, you're highlighting the naivete of the plain assignment... Um... yeah, I suppose I could update.... I don't see any upsides though?

Copy link
Member

@umarcor umarcor May 20, 2020

Choose a reason for hiding this comment

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

Yes. Sorry, I was in a hurry.

The main point is that there are no obvious downsides, and it can be useful. Some use cases:

  • CC=g++ or CC=clang are valid options. For example, the latter is used to build GHDL on macOS in CI: https://github.com/ghdl/ghdl/blob/master/dist/ci-run.sh#L399
  • Tristan uses GHDL= very frequently in order to run testsuites with temporal builds of GHDL. This repo is not a test suite, but we might want to make it easier for him and for us to test examples with non-default locations and names of GHDL binaries.
  • By the same token, we might want to easily test some examples with various versions of the standard. Some are for 93 only, others for 08 only, but most of them should work with any version.

Actually, we might want to add these comments to the README, along with your explanation above about how make is expected to be used. If you feel that it does not fit in the README, we can create a separate file for most new/unexperienced users.

vhpidirect/arrays/intvector/csized/makefile Outdated Show resolved Hide resolved
with:
msystem: MINGW64
update: true
install: mingw-w64-x86_64-python-pip
Copy link
Member

Choose a reason for hiding this comment

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

The failure of this task should be fixed by adding make here: install: mingw-w64-x86_64-python-pip make.

task: [
vhpidirect/arrays/matrices/vunit_axis_vcs,
]
win-stable:
Copy link
Member

Choose a reason for hiding this comment

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

The failure of this task might be legit. The make that is available in the PATH was not explicitly installed by us. It seems to be preinstalled from Chocolatey. We might investigate if we can make it work (e.g. check_call(['make', 'run'], shell=True, cwd=str(self.vhpidirect / 'arrays' / 'intvector'))) or whether we need to use eine/setup-msys2 here too.

I'd say that v0.37 windows releases of GHDL do require a MINGW environment. However, it has been surprising for me that just extracting the zipfile was working fine until now. Maybe GHDL was picking the MINGW libs of Git for Windows, which is installed by default.

Copy link
Member

Choose a reason for hiding this comment

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

Glad to see it worked. It would have been a pain to fix win-stable otherwise...

README.md Outdated
All makefile rules can be called by specifying the target (argument left of the rule's ':') after make: e.g. `make run`

Make echoes each command before executing it, unless the line starts with `@`. All lines are echoed to the terminal, excepting `echo` calls.
This provides transparency on the commans being executed.
Copy link
Member

Choose a reason for hiding this comment

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

Typo: commands.

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

Successfully merging this pull request may close these issues.

2 participants