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

Removal of "No CMake" build option #1299

Merged
merged 68 commits into from
Oct 18, 2022
Merged

Removal of "No CMake" build option #1299

merged 68 commits into from
Oct 18, 2022

Conversation

petervdonovan
Copy link
Collaborator

@petervdonovan petervdonovan commented Jul 20, 2022

This branch deletes over 500 LOC and breaks the "no CMake" build option that directly invokes the C compiler. The corresponding PRs in reactor-c and reactor-c-py should be merged first.

It's a bit of a work-in-progress, but I wish to rush this into master to mitigate conflicts. I promise to help resolve any conflicts that result in the fed-gen branch.

The #line directives make debugging impossible and obfuscate the generated
code. I should re-enable them before merging to main.
Just to see what would happen.
We have informally discussed setting the margins in this way. I decided
not to wait any longer for the opportune moment to make this change.
Do not use a C++ compiler for the runtime. Only use a C++ compiler for
the generated code.
This is a work in progress. In particular, setup.py is broken.
This removes interaction with Python package management systems such
as Pip from the LF build process. We do not need to install the C
extension module in the user's environment because it is sufficient
to have a naked .so or .dll sitting in the `src-gen` directory.
This includes an attempt to pass federated and docker tests.
This also keeps the file names that existed in master in
org.lflang.generator.c and org.lflang.generator.python.
This follows up on a conversation with @arengarajan99 and Marten about
the Arduino tests. The rationale is that soon, our Arduino support will
be entirely different anyway, and that we can tolerate a temporary
regression in the interim as a shortcut to save person-hours.

Obviously I take responsibility if we have to scramble to get support
back in time for a release.
This is a hack! The fact that it is needed is a reflection of the fact
that the changes introduced by this PR leave the build system sensitive
to idiosyncrasies in the way Python is installed and discovered on the
system.
@cmnrd
Copy link
Collaborator

cmnrd commented Oct 17, 2022

In this case it should be helpful to get the verbose output of cmake and compare the executed commands in CI to a local run.

@petervdonovan
Copy link
Collaborator Author

I did as Christian suggested and can confirm that CMake is discovers a different version of Python than what is discovered by which python. To be specific, invocations of gcc include the option -isystem /usr/local/opt/python@3.10/Frameworks/Python.framework/Versions/3.10/include/python3.10 whereas when I SSH into the runner I see the following:

Mac-1666038353816:lingua-franca runner$ which python3
/usr/local/bin/python3
Mac-1666038353816:lingua-franca runner$ ls -al /usr/local/bin/python3
lrwxr-xr-x  1 runner  admin  40 Oct 11 00:31 /usr/local/bin/python3 -> ../Cellar/python@3.10/3.10.7/bin/python

Basically, one Python is in the Cellar, and the other is not.

There are ways to fix this, e.g., by explicitly specifying the path to Python when we invoke CMake.; furthermore, I know where in the code base this change would have to be made. However, there are a few reasons to stop here:

  • Every build environment is different, and an end user can always find ways to break their build by setting up their environment in special ways. Arguably, whether this is a bug in the build system or in the environment is a matter of perspective.
  • It is possible to work around this issue by using a virtual environment (which most Python programmers should do anyway).
  • This bug has never been observed except in CI, and it could be very rare.
  • It is not clear to me that other ways of integrating Python with CMake are recommended over the one I used; therefore, it is not obvious to me that fixing this bug would reduce the total number of bugs that we have.

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

This looks great! Congrats on finally getting this PR to pass the tests!! 🎆 I left some comments/requests. I wonder how much trouble these changes will give us in fed-gen... Perhaps you could assist with the merge? I haven't reviewed the changes in reactor-c yet. I will do that next.

org.lflang/src/org/lflang/ASTUtils.java Outdated Show resolved Hide resolved
org.lflang/src/org/lflang/ASTUtils.java Outdated Show resolved Hide resolved
org.lflang/src/org/lflang/TargetProperty.java Show resolved Hide resolved
org.lflang/src/org/lflang/util/FileUtil.java Show resolved Hide resolved
@lhstrh
Copy link
Member

lhstrh commented Oct 18, 2022

Looks like there is no accompanying PR for reactor-c-py?

petervdonovan and others added 2 commits October 18, 2022 13:20
Co-authored-by: Marten Lohstroh <marten@berkeley.edu>
@petervdonovan petervdonovan changed the title Remove "No CMake" build option. Removal of "No CMake" build option. Oct 18, 2022
@petervdonovan petervdonovan changed the title Removal of "No CMake" build option. Removal of "No CMake" build option Oct 18, 2022
@petervdonovan petervdonovan merged commit 18892d7 into master Oct 18, 2022
@petervdonovan petervdonovan deleted the c-refactoring2 branch October 18, 2022 20:57
@lhstrh lhstrh added the refactoring Code quality enhancement label Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Code quality enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants